Skip to content

Conversation

@g0w6y
Copy link

@g0w6y g0w6y commented Dec 22, 2025

This PR fixes a critical path traversal vulnerability in the fetchRemote method of the Files class. Previously, remote filenames containing traversal sequences (e.g., ../../) were resolved without validation, allowing files to be written outside the intended project directory.

Changes

Established a Security Boundary: Resolved the absolute path of the contentDir to serve as a "jail" for file operations.

Path Normalization: Used path.resolve to normalize remote filenames and extensions into absolute local paths.

Boundary Validation: Implemented a check to ensure that the resolvedPath starts with the absoluteContentDir followed by the platform-specific path separator.

Security Error Handling: Added logic to throw a Security Error if a file resolution attempt falls outside the project directory, halting potentially malicious write operations.

Impact This fix prevents an attacker with control over a remote Apps Script project from performing arbitrary file writes on a user's local machine during a clasp pull or clasp clone. This effectively neutralizes a potential Remote Code Execution (RCE) vector.

Added security check to prevent file path traversal.
@google-cla
Copy link

google-cla bot commented Dec 22, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@g0w6y
Copy link
Author

g0w6y commented Jan 10, 2026

Any Update ?

@sqrrrl
Copy link
Member

sqrrrl commented Jan 20, 2026

Appreciate the fix, just need the CLA signed

const resolvedPath = path.resolve(contentDir, `${f.name}${ext}`);

// SECURITY CHECK: Ensure the resolved path remains inside the absoluteContentDir
if (!resolvedPath.startsWith(absoluteContentDir + path.sep) && resolvedPath !== absoluteContentDir) {
Copy link
Member

Choose a reason for hiding this comment

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

This still allows some paths outside the content dir. The path won't have the trailing /, small edge cases where a parallel dirs share the same prefix.

/foo/bar - content dir
/foo/bar1 - not content dir

../bar1/file.js would resolve to /foo/bar1/file.js, which would pass the startsWith '/foo/bar' check.

Tighter check:

function isParent(parentPath, childPath) {
  const relative = path.relative(parentPath, childPath);
  return relative && !relative.startsWith('..') && !path.isAbsolute(relative);
}

Copy link
Member

@sqrrrl sqrrrl left a comment

Choose a reason for hiding this comment

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

Suggest tightening up the check. It's mostly good, just a small edge case it misses.

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