Skip to content

fix: prevent symlink traversal during retrieve (partial-delete and writes) @W-24124120@ @W-24138711@ - #1833

Open
WillieRuemmele wants to merge 4 commits into
mainfrom
wr/symlinkPartialDelete
Open

WillieRuemmele wants to merge 4 commits into
mainfrom
wr/symlinkPartialDelete

Conversation

@WillieRuemmele

@WillieRuemmele WillieRuemmele commented Sep 9, 2026

Copy link
Copy Markdown
Member

Summary

  • Partial-delete path (W-24124120): Use lstatSync instead of statSync when checking if component content paths are directories, preventing symlinked content directories from entering the partial-delete flow. Filter out symlinks within content directories before deletion and add defense-in-depth symlink check in deleteFilePath.
  • Write/overwrite path (W-24138711): Add findSymlinkOnPath check in StandardWriter._write() before every file write and delete during retrieve, covering all metadata types. If any path segment between the project root and the output file is a symbolic link, the operation is rejected with a clear error.
  • Shared utility: Extract findSymlinkOnPath from StaticResourceMetadataTransformer to fileSystemHandler.ts so it can be reused across the partial-delete, write, and static resource extraction paths.

Security Context

During sf project retrieve start, two distinct I/O paths followed symbolic links without validation:

  1. Partial-delete — for supportsPartialDelete types (e.g. DigitalExperienceBundle), the logic would follow symlinks when enumerating and deleting local-only content. If a content directory was a symlink to an external path, retrieve could delete files outside the Salesforce project.
  2. File writes — the StandardWriter that writes all retrieved metadata to disk would follow symlinks in the output path. A symlinked file or directory inside the project (e.g. planted in a shared Git repo) could redirect writes to overwrite files outside the workspace.

Both attack vectors require a crafted project (e.g. a malicious Git repo with preserved symlinks) that a victim clones and retrieves against.

@W-24124120@ @W-24138711@

Proof of Work

  • Tests: 1103+ passing, 0 failing
  • Lint: clean
  • Type check: clean
  • New tests: 11 total (4 in retrieveExtract.test.ts, 3 in streams.test.ts, 4 in fileSystemHandler.test.ts)

Test plan

  • Verify partial-delete still works correctly for non-symlinked DigitalExperienceBundle content
  • Verify symlinked content directories are excluded from partial-delete processing
  • Verify symlinked files within content directories are not deleted
  • Verify normal retrieve writes work for all metadata types (no false positives)
  • Verify retrieve rejects writes through symlinked files in the project
  • Verify retrieve rejects writes through symlinked directories in the project

…4124120@

Use lstatSync instead of statSync to reject symlinked content directories
from partial-delete processing. Filter out symlinks within content directories
before deletion. Add defense-in-depth symlink check in deleteFilePath.
@WillieRuemmele

WillieRuemmele commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

QA Instructions

Setup

  1. Create a Salesforce project with a DigitalExperience site (or use an existing one)
  2. Retrieve the site: sf project retrieve start -m DigitalExperienceBundle
  3. Verify you have a content directory under force-app/main/default/digitalExperiences/site/<siteName>/
  4. Also have at least one Apex class or other metadata type for the write-path tests

Test 1: Symlinked content directory is NOT followed during partial-delete (W-24124120)

  1. Create an external directory with some files:
    mkdir -p /tmp/external-target
    echo "important data" > /tmp/external-target/file1.txt
    echo "critical config" > /tmp/external-target/file2.txt
  2. Replace a content directory with a symlink to the external directory:
    # Pick a content dir, e.g. a view under the site bundle
    CONTENT_DIR="force-app/main/default/digitalExperiences/site/<siteName>/sfdc_cms__view/<viewName>"
    rm -rf "$CONTENT_DIR"
    ln -s /tmp/external-target "$CONTENT_DIR"
  3. Run a retrieve that would trigger partial-delete:
    sf project retrieve start -m DigitalExperienceBundle
  4. Expected: The symlinked directory is skipped — files in /tmp/external-target/ are NOT deleted. The retrieve completes without errors.

Test 2: Normal partial-delete still works (no false positives)

  1. Start with a clean retrieve of a DigitalExperience site (no symlinks)
  2. Add a local-only file inside a content directory:
    echo "local only" > "force-app/main/default/digitalExperiences/site/<siteName>/sfdc_cms__view/<viewName>/localFile.txt"
  3. Run: sf project retrieve start -m DigitalExperienceBundle
  4. Expected: The local-only file (localFile.txt) IS deleted as part of normal partial-delete behavior — the fix does not break the existing flow for real directories.

Test 3: Symlinked file blocks retrieve writes (W-24138711)

  1. Create an external file to serve as the symlink target:
    echo "external secret" > /tmp/external-secret.txt
  2. Replace an Apex class file with a symlink:
    APEX_FILE="force-app/main/default/classes/MyClass.cls"
    rm "$APEX_FILE"
    ln -s /tmp/external-secret.txt "$APEX_FILE"
  3. Retrieve that class:
    sf project retrieve start -m ApexClass:MyClass
  4. Expected: The retrieve fails with an error containing "symbolic link" — the external file is NOT overwritten. Verify /tmp/external-secret.txt still contains "external secret".

Test 4: Symlinked directory blocks retrieve writes (W-24138711)

  1. Create an external directory:
    mkdir -p /tmp/external-dir
    echo "do not overwrite" > /tmp/external-dir/important.txt
  2. Replace the classes directory with a symlink:
    mv force-app/main/default/classes force-app/main/default/classes.bak
    ln -s /tmp/external-dir force-app/main/default/classes
  3. Retrieve Apex classes:
    sf project retrieve start -m ApexClass
  4. Expected: The retrieve fails with an error containing "symbolic link" — nothing is written to /tmp/external-dir/. Verify important.txt is unchanged.
  5. Restore the real directory:
    rm force-app/main/default/classes
    mv force-app/main/default/classes.bak force-app/main/default/classes

Test 5: Normal retrieve writes work (no false positives)

  1. Ensure no symlinks exist in the project directory
  2. Run: sf project retrieve start -m ApexClass
  3. Expected: Retrieve completes normally. All files are written as expected.

Extract findSymlinkOnPath to shared fileSystemHandler utility and add
symlink traversal protection to StandardWriter._write() — covers both
the write and delete paths for all metadata types during retrieve.
@WillieRuemmele
WillieRuemmele requested a review from a team as a code owner September 11, 2026 19:06
@WillieRuemmele WillieRuemmele changed the title fix: prevent symlink following during partial-delete in retrieve @W-24124120@ fix: prevent symlink traversal during retrieve (partial-delete and writes) @W-24124120@ @W-24138711@ Sep 11, 2026
Resolves conflicts in streams.ts (combined symlink + path traversal
imports), streams.test.ts (kept both symlink and path traversal tests),
and eslint-suppressions.json (combined _write dangle counts).
(fr: FileResponseSuccess): FileResponseSuccess => {
if (fr.filePath) {
if (isSymlinkSync(fr.filePath)) {
logger.debug(`Skipping delete of symlink ${fr.filePath} to prevent modification of files outside the project.`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

logger.warn might me more appropriate for this.

@iowillhoit

Copy link
Copy Markdown
Contributor

🔴 Partial-delete still deletes outside the project when a bundle ancestor dir is a symlink (W-24124120 not fully closed)

The delete path only guards the leaf (the last segment of the path), so the containment boundary can still be escaped by a symlinked ancestor directory.

The gap

  • handlePartialDeleteMerges runs before the convert/write step, and its symlink guards are leaf-only:
  • supportsPartialDeleteAndHasContentlstatSync(comp.content) only refuses to follow the final segment
  • the candidate filter and deleteFilePathisSymlinkSync(join(contentPath, fileName)) only checks the direct child

Neither inspects an ancestor of the content root. Local resolution uses statSync().isDirectory() (src/resolve/treeContainers.ts:119), which follows symlinks, so a DEB whose digitalExperiences ancestor is a symlink to an external directory resolves normally, its children end in real-file leaves, and partial-delete rmSyncs the external, local-only files through it.

The write-path guard would catch the symlinked ancestor (it walks all segments), but it throws only later in StandardWriter._write. By then the delete has already happened and isn't rolled back.

Repro (confirmed against this branch)

End-to-end through the real extract: a source DEB project where digitalExperiences/ is relocated outside the project and replaced with a symlink; a retrieve zip missing one content file so it's treated as local-only.

victim exists BEFORE : true
extract threw        : Error_retrieve_symlinkError   // write guard fires…
victim exists AFTER  : false                          // …but the external file is already gone
symlink still present: true

The delete precedes the write abort, so the write-path guard does not protect against this. (I have a failing mocha I can share)

Suggested fix

Give the delete path the same full-path protection the write path already has. In deleteFilePath (or as the filter feeding it), reject any candidate where an ancestor segment is a symlink: e.g. await findSymlinkOnPath(packageRoot, fr.filePath) is truthy, or do a realpath-based containment check against the package root before rmSync. That gives delete/write parity and matches the PVR's explicit ask: "Resolve the real path of every local deletion candidate and verify it remains inside the expected package/project root before any delete operation."

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.

3 participants