Skip to content

CodeQL 2: fix(security): close remaining CodeQL security alerts#190

Open
rlorenzo wants to merge 1 commit into
mainfrom
codeql/2-security-alerts
Open

CodeQL 2: fix(security): close remaining CodeQL security alerts#190
rlorenzo wants to merge 1 commit into
mainfrom
codeql/2-security-alerts

Conversation

@rlorenzo
Copy link
Copy Markdown
Contributor

@rlorenzo rlorenzo commented May 13, 2026

Summary

Closes the 6 remaining CodeQL alerts in the security family that aren't already addressed by PR #184 (CMS path-injection) or PR #189 (generated-code exclusions).

  • scripts/lint-any.js - switch temp-file creation from a predictable name in os.tmpdir() to fs.mkdtempSync (private mode-0700 directory, cryptographic suffix). Cleanup moved into a returned callback so the directory is removed, not just a single file. Closes 1× js/insecure-temporary-file.
  • scripts/lib/lint-staged-common.js:321 - drop duplicated \\ in the Windows drive-letter character class. Semantically a no-op ([\\/] and [\/] match the same characters). Closes 1× js/regex/duplicate-in-character-class.
  • web/Program.cs (×3), web/ViteProxyHelpers.cs - migrate Path.CombinePath.Join. All four call sites pass relative second args today, so the silent-drop edge case the rule warns about can't actually fire; Path.Join lacks the footgun. Closes 4× cs/path-combine.

Context

Second in the CodeQL N: cleanup series (after #189). Independent of #184/#189.

Test plan

  • npm run test - 1946 backend + 749 frontend passing
  • npm run lint - clean on changed files (pre-existing warnings unrelated)
  • npm run verify:build - passes
  • CodeQL workflow on this PR shows the 6 listed alerts closed

Summary by CodeRabbit

  • Bug Fixes
    • Improved path handling for cross-platform compatibility, ensuring consistent behavior on Windows and Unix-based systems.
    • Enhanced temporary file cleanup for linting processes to prevent resource leaks during parallel execution.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

📝 Walkthrough

Walkthrough

Windows path regex simplified for lint-staged paths; linter temp file handling refactored to use dedicated temp directories with cleanup closures instead of single-file unlinking; .NET path construction standardized from Path.Combine to Path.Join across three middleware files.

Changes

Linting infrastructure refactor

Layer / File(s) Summary
Windows path detection regex
scripts/lib/lint-staged-common.js
Windows drive-path detection regex in sanitizeFilePath updated to match backslash or forward slash using a simplified character class.
Temp directory and cleanup function refactor
scripts/lint-any.js
buildScriptArgs() replaces single temp-file creation with fs.mkdtempSync temp directory and returns { scriptArgs, cleanup } object with a closure that recursively removes the temp directory.
Caller integration of cleanup closure
scripts/lint-any.js
runLinter() and runLinterAsync() updated to destructure and invoke the cleanup() closure after child process completion, replacing inline fs.unlinkSync logic in both sync and async paths.

Path API standardization

Layer / File(s) Summary
Path.Combine to Path.Join across .NET codebase
web/Program.cs, web/ViteProxyHelpers.cs
Three locations updated to use Path.Join instead of Path.Combine: .env.local environment file path, Vue SPA middleware base paths for both default files and static assets, and Vite proxy fallback path construction.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • bsedwards
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title addresses CodeQL security alerts, but is overly broad and uses jargon ('CodeQL 2', 'security alerts') without specifying what was actually changed. Consider a more specific title that describes the actual changes, e.g., 'Replace Path.Combine with Path.Join and fix temp file creation' or 'Fix CodeQL security alerts: path handling and temp files'.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly details six specific CodeQL security alerts being closed with precise file locations and the technical rationale for each fix.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codeql/2-security-alerts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 13, 2026

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.96%. Comparing base (f52e63b) to head (a6ad40f).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
web/Program.cs 0.00% 3 Missing ⚠️
web/ViteProxyHelpers.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #190   +/-   ##
=======================================
  Coverage   42.96%   42.96%           
=======================================
  Files         877      877           
  Lines       51468    51468           
  Branches     4802     4802           
=======================================
  Hits        22113    22113           
  Misses      28831    28831           
  Partials      524      524           
Flag Coverage Δ
backend 43.04% <0.00%> (ø)
frontend 41.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rlorenzo
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@scripts/lint-any.js`:
- Around line 317-323: The temp directory created by fs.mkdtempSync (tempDir)
can leak if fs.writeFileSync(tempFile, ...) throws because cleanup is only
assigned afterwards; change the logic so that cleanup is set immediately after
creating tempDir (or wrap the mkdtempSync + writeFileSync sequence in a
try/catch) and in the catch remove tempDir (fs.rmSync(tempDir, { recursive:
true, force: true })) before rethrowing the error, ensuring tempDir/tempFile are
always cleaned up even when fs.writeFileSync fails; update references around
tempDir, tempFile, fs.mkdtempSync, fs.writeFileSync, and cleanup accordingly.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 8c8bca24-88db-426b-96d8-b7be589bc90b

📥 Commits

Reviewing files that changed from the base of the PR and between 38de1ad and 3013f3c.

📒 Files selected for processing (4)
  • scripts/lib/lint-staged-common.js
  • scripts/lint-any.js
  • web/Program.cs
  • web/ViteProxyHelpers.cs

Comment thread scripts/lint-any.js
- scripts/lint-any.js: replace predictable temp-file name in os.tmpdir()
  with fs.mkdtempSync (private mode-0700 dir, cryptographic suffix) so
  the file inside cannot be predicted/preempted by another local
  process. Cleanup moved into a returned callback to remove the dir
  rather than just unlinking a single file.
- scripts/lib/lint-staged-common.js: drop duplicated backslash in
  Windows drive-letter character class (semantically a no-op).
- web/Program.cs, web/ViteProxyHelpers.cs: migrate four Path.Combine
  calls to Path.Join per CLAUDE.md convention; avoids the silent-drop
  behavior the rule flags.

Closes CodeQL alerts: js/insecure-temporary-file (1),
js/regex/duplicate-in-character-class (1), cs/path-combine (4).
@rlorenzo rlorenzo force-pushed the codeql/2-security-alerts branch from 3013f3c to a6ad40f Compare May 13, 2026 15:49
@rlorenzo rlorenzo requested a review from bsedwards May 13, 2026 16:04
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.

2 participants