Skip to content

Safe filenames#8480

Draft
Amndeep7 wants to merge 15 commits into
mainfrom
safe_filenames
Draft

Safe filenames#8480
Amndeep7 wants to merge 15 commits into
mainfrom
safe_filenames

Conversation

@Amndeep7

@Amndeep7 Amndeep7 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor
  • Fixes a bug in resolvesafechild where it didn't check if the final filename was a symlink
  • Simplified basename implementation since the windows version of basename could handle both OSes to the degree that we needed
  • Added file sanitization support
  • Made as many places as possible use the helper functions so as to have safe filenames without path traversal or symlink issues
  • Improved and added more tests

Despite all of these improvements, we still cannot reliably get around TOCTOU issues due to lack of support from the runtime in directly interfacing with filehandles as opposed to paths. See: https://snyk.io/articles/safe-path-handling/

Amndeep7 added 7 commits June 24, 2026 18:14
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
…endently create their tempdirs instead of generating them one at a time which could lead to race conditions when run in parallel like vitest is currently configured to do

Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
… only checks if the intermediate directories are symlinks not the final path - this test is currently failing

Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
…esafechild

Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
@Amndeep7 Amndeep7 marked this pull request as ready for review June 25, 2026 07:43
@Amndeep7 Amndeep7 requested review from aaronlippold and wdower June 25, 2026 07:46
@Amndeep7 Amndeep7 marked this pull request as draft June 25, 2026 08:01
… more characters to handle all of powershell + cmd.exe

Signed-off-by: Amndeep Singh Mann <amann@mitre.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens path and filename handling across the CLI by introducing a centralized “safe filename” helper, extending symlink protections in safe path resolution, and migrating many write/copy operations to use these helpers to reduce path traversal and symlink-following risks.

Changes:

  • Add safeFilename() (backed by sanitize-filename) and update basename() behavior for cross-OS separator handling.
  • Strengthen resolveSafeChild() to reject symlink targets (not just symlink parents) and apply safe path/filename helpers across multiple commands.
  • Expand/adjust tests around basename, safeFilename, and resolveSafeChild; add sanitize-filename dependency.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/utils/tests/global.test.ts Adds coverage for basename, safeFilename, and resolveSafeChild symlink scenarios; refactors temp dir cleanup.
test/commands/generate/update_controls4delta.test.ts Adds a Windows-only regression test for rejecting shell metacharacters in paths.
src/utils/global.ts Introduces safeFilename, updates basename to use path.win32.basename, and strengthens resolveSafeChild symlink protections.
src/utils/emasser/utilities.ts Uses resolveSafeChild + safeFilename when saving files.
src/commands/generate/update_controls4delta.ts Uses resolveSafeChild + safeFilename for control file path handling.
src/commands/generate/spreadsheet2inspec_stub.ts Routes output writes through resolveSafeChild and uses safeFilename for generated control filenames.
src/commands/generate/inspec_profile.ts Uses safeFilename/resolveSafeChild for generated profile/control outputs and related files.
src/commands/generate/delta.ts Uses safeFilename/resolveSafeChild for generated control output paths and some copied files.
src/commands/generate/ckl_metadata.ts Uses resolveSafeChild and safeFilename for interactive output path construction.
src/commands/convert/trivy2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/convert/splunk2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/convert/snyk2hdf.ts Uses resolveSafeChild and safeFilename for output filenames, including multi-output behavior.
src/commands/convert/prowler2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/convert/prisma2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/convert/nessus2hdf.ts Uses resolveSafeChild and safeFilename for output filenames, including multi-output behavior.
src/commands/convert/msft_secure2hdf.ts Uses resolveSafeChild and safeFilename for output filenames for multiple reports.
src/commands/convert/ionchannel2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/convert/index.ts Applies resolveSafeChild and safeFilename to several detected-type output paths/filenames.
src/commands/convert/hdf2html.ts Uses shared basename helper for input file display names.
src/commands/convert/hdf2csv.ts Uses resolveSafeChild + safeFilename for interactive output path building.
src/commands/convert/hdf2asff.ts Uses resolveSafeChild + safeFilename for output file paths.
src/commands/convert/conveyor2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/convert/ckl2poam.ts Uses resolveSafeChild + safeFilename for output .xlsm path generation.
src/commands/convert/asff2hdf.ts Writes outputs via resolveSafeChild and validates filenames with safeFilename.
src/commands/attest/apply.ts Uses resolveSafeChild + safeFilename when writing multi-output attestations.
package.json Adds sanitize-filename dependency.
package-lock.json Locks sanitize-filename and transitive dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

'-X', path.resolve('./test/sample_data/xccdf/stigs/Windows_Server_2019_V3R2_xccdf.xml'),
'-c', tempControlsDir,
'--no-backupControls',
])).rejects.toThrow(/Unsafe shell characters/);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the test first, will be adding in the implementation afterwards

Comment thread src/utils/global.ts Outdated
Comment thread src/utils/emasser/utilities.ts Outdated

// Save to file
const outDir = path.join(dir, filename);
const outDir = resolveSafeChild(dir, safeFilename(filename));
@wdower

wdower commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Sonar is mad about "5.2% duplicated code," which I assume is because you are pretty mechanically adding these helpers into a bunch of areas (but Sonar makes it hard to tell if that's what it's mad about).

Fix test errors and I'm good.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@Amndeep7

Copy link
Copy Markdown
Contributor Author

@wdower yeah the code dup stuff is basically just cause i'm adding in a lot of use of these helper functions which look pretty repetitive across the codebase since we interact with the filesystem across a lot of the codebase

Amndeep7 added 6 commits June 25, 2026 19:48
…irectory where the results will be saved

Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
…tion

Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
@sonarqubecloud

sonarqubecloud Bot commented Jul 6, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
3.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

Security: shell injection via unsanitized path in generate delta and update_controls4delta

3 participants