security: ignore symlinks by default in fsAsync.readdirRecursive#10477
security: ignore symlinks by default in fsAsync.readdirRecursive#10477wecbaiyk-blip wants to merge 2 commits intofirebase:mainfrom
Conversation
readdirRecursive supports an `ignoreSymlinks` option but defaults it to false. Three of the four current callers omit the option entirely: - src/deploy/apphosting/util.ts createLocalBuildTarArchive (line 32) - src/deploy/apphosting/util.ts createSourceDeployArchive (line 98) - src/deploy/functions/prepareFunctionsUpload.ts (line 117) A symlink in the source tree pointing at /proc/self/environ, ~/.config/gcloud/application_default_credentials.json, ~/.ssh/id_rsa, or similar will be followed during App Hosting and Cloud Functions deploys and its target's contents uploaded to whatever destination the deploy targets. Largest exposure is CI workflows that extract attacker-supplied tarballs. This change flips the default to true. Callers that legitimately need symlink-following can opt in explicitly with `ignoreSymlinks: false`. The existing safe caller (src/archiveDirectory.ts) keeps working unchanged because it passes ignoreSymlinks: true explicitly. Tests updated to assert the new safer default and the explicit-opt-out path.
|
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. |
There was a problem hiding this comment.
Code Review
This pull request updates the readdirRecursive function to ignore symbolic links by default to prevent security vulnerabilities such as path traversal during deployment. The changes include enhanced JSDoc documentation explaining the security rationale, the addition of debug logging when symlinks are encountered, and updated unit tests to verify the new default behavior and explicit options. I have no feedback to provide.
|
@googlebot I signed it! |
Summary
fsAsync.readdirRecursiveis the directory walker used by every firebase-tools deploy that enumerates a source directory. It already supports anignoreSymlinksoption, but that option defaults tofalse. Three of the four current callers omit the option entirely, so symlinks in the source tree are followed during App Hosting deploys (createLocalBuildTarArchive,createSourceDeployArchive) and Cloud Functions deploys (prepareFunctionsUpload).This PR flips the default to
true. Callers that legitimately need symlink-following can opt in explicitly withignoreSymlinks: false.Why
A symlink inside the source tree pointing at
/proc/self/environ,~/.config/gcloud/application_default_credentials.json,~/.ssh/id_rsa, or similar will be followed by the recursion and its target''s contents uploaded to whatever destination the deploy targets (Firebase Hosting public bucket, App Hosting build bucket, Cloud Functions GCS staging bucket, etc.). The attack surface is largest in CI workflows that extract attacker-supplied tarballs (e.g. PR-preview deploys) but applies anywhere a deploy operates on attacker-influenced files.The one caller that already does the right thing (
src/archiveDirectory.ts) keeps working unchanged because it explicitly passesignoreSymlinks: true.Caller audit:
ignoreSymlinks: true?src/archiveDirectory.ts:79src/deploy/apphosting/util.ts:32(createLocalBuildTarArchive)src/deploy/apphosting/util.ts:98(createSourceDeployArchive)src/deploy/functions/prepareFunctionsUpload.ts:117Changes
src/fsAsync.ts: defaultignoreSymlinkstotrue; emit adebug-level log when a symlink is dropped; add security-rationale JSDoc on the option and the function.src/fsAsync.spec.ts: replace the existing single symlink test with two - one asserting the new safer default, one asserting thatignoreSymlinks: falsestill includes symlinks for users who explicitly opt in.No changes to call sites are required for the security fix: the three currently-unsafe callers (
apphosting/util.tsx2,prepareFunctionsUpload.ts) inherit the safer default for free.Backward compatibility
The behavior changes only for callers who (a) didn''t pass
ignoreSymlinksat all and (b) have a source tree that contains symlinks they want followed during deploy. Both conditions together are a narrow population; the typical project tree is unaffected. Users who rely on symlink-following can keep the old behavior by passingignoreSymlinks: falseexplicitly.If the maintainers prefer a warn-then-deprecate path instead (default stays
false, log a warning when a symlink is encountered, flip the default in a future major), happy to adjust.Testing
Two new tests in
src/fsAsync.spec.ts: