Skip to content

security: drop symlinks from Hosting deploy upload list #10478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wecbaiyk-blip wants to merge 1 commit into
base: main
Choose a base branch
from
+133 −2
Open

security: drop symlinks from Hosting deploy upload list #10478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions src/listFiles.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { expect } from "chai";
import * as crypto from "crypto";
import * as fs from "fs";
import * as os from "os";
import * as path from "path";
import { rmSync } from "node:fs";
Comment on lines +2 to +6
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

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

medium

Inconsistent use of built-in module imports. It is recommended to use the node: prefix for all built-in modules for clarity and to follow modern Node.js conventions. Additionally, mixing namespace imports (import * as fs) with named imports (import { rmSync }) for the same module is redundant when the named export is available on the namespace object.

Suggested change
import * as crypto from "crypto";
import * as fs from "fs";
import * as os from "os";
import * as path from "path";
import { rmSync } from "node:fs";
import * as crypto from "node:crypto";
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";


import { listFiles } from "./listFiles";
import { FIXTURE_DIR } from "./test/fixtures/ignores";
Expand Down Expand Up @@ -30,4 +35,84 @@ describe("listFiles", () => {
"present/index.html",
]);
});

describe("symlink handling", () => {
let tmpRoot = "";
let outsideTarget = "";

before(() => {
tmpRoot = path.join(os.tmpdir(), "fb-tools-listFiles-" + crypto.randomBytes(6).toString("hex"));
fs.mkdirSync(tmpRoot, { recursive: true });
// Two regular files inside the public dir.
fs.writeFileSync(path.join(tmpRoot, "index.html"), "<!doctype html>");
fs.mkdirSync(path.join(tmpRoot, "static"));
fs.writeFileSync(path.join(tmpRoot, "static", "app.js"), "// app");
// A target outside the public dir to simulate the credential-leak case.
outsideTarget = path.join(os.tmpdir(), "fb-tools-listFiles-leak-" + crypto.randomBytes(6).toString("hex"));
fs.writeFileSync(outsideTarget, "SECRET-CONTENT");
});

after(() => {
rmSync(tmpRoot, { recursive: true, force: true });
rmSync(outsideTarget, { force: true });
});
Comment on lines +55 to +58
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

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

medium

The after hook should safely handle cases where tmpRoot or outsideTarget might be empty strings (e.g., if the before hook fails before they are assigned). Calling rmSync("") can throw an error, which might obscure the actual failure in the test report. Adding truthy checks makes the cleanup more robust.

    after(() => {
      if (tmpRoot) {
        fs.rmSync(tmpRoot, { recursive: true, force: true });
      }
      if (outsideTarget) {
        fs.rmSync(outsideTarget, { force: true });
      }
    });


it("excludes a symlink-to-file at the top level (security: prevents leaking files outside source tree)", () => {
const linkPath = path.join(tmpRoot, "leak");
try {
fs.symlinkSync(outsideTarget, linkPath);
const result = listFiles(tmpRoot);
expect(result.sort()).to.have.members(["index.html", "static/app.js"]);
expect(result).to.not.include("leak");
} finally {
try {
fs.unlinkSync(linkPath);
} catch {
/* ignore */
}
}
});

it("excludes a symlink-to-file nested inside the source tree", () => {
const linkPath = path.join(tmpRoot, "static", "leak");
try {
fs.symlinkSync(outsideTarget, linkPath);
const result = listFiles(tmpRoot);
expect(result.sort()).to.have.members(["index.html", "static/app.js"]);
expect(result).to.not.include("static/leak");
} finally {
try {
fs.unlinkSync(linkPath);
} catch {
/* ignore */
}
}
});

it("does not descend into symlinked directories", () => {
const outsideDir = path.join(
os.tmpdir(),
"fb-tools-listFiles-leakdir-" + crypto.randomBytes(6).toString("hex"),
);
fs.mkdirSync(outsideDir);
fs.writeFileSync(path.join(outsideDir, "secret"), "SECRET-CONTENT");
const linkDirPath = path.join(tmpRoot, "linked-dir");
try {
fs.symlinkSync(outsideDir, linkDirPath, "dir");
const result = listFiles(tmpRoot);
expect(result.sort()).to.have.members(["index.html", "static/app.js"]);
// No file from the linked dir should appear, regardless of stat behavior.
for (const r of result) {
expect(r.startsWith("linked-dir")).to.equal(false);
}
} finally {
try {
fs.unlinkSync(linkDirPath);
} catch {
/* ignore */
}
rmSync(outsideDir, { recursive: true, force: true });
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

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

medium

Use fs.rmSync for consistency with the namespace import recommended above.

Suggested change
rmSync(outsideDir, { recursive: true, force: true });
fs.rmSync(outsideDir, { recursive: true, force: true });

}
});
});
});
50 changes: 48 additions & 2 deletions src/listFiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,58 @@
import { lstatSync } from "fs";
import { join } from "path";
import { sync } from "glob";

import { logger } from "./logger";

/**
* Recursively list deployable files under `cwd`.
*
* **Security: symlinks are excluded by default.**
*
* Hosting deploys take this list, read each entry with `fs.readFile*`
* (which follows symlinks at the OS layer), and uploads the bytes to a
* Firebase Hosting site. If the source tree contains a symlink such as
* `public/leak -> /proc/self/environ` or
* `public/leak -> ~/.config/gcloud/application_default_credentials.json`,
* the target's contents would otherwise end up published on a
* Firebase-hosted public URL.
*
* The largest exposure is CI workflows that extract attacker-supplied
* tarballs into the public directory before invoking `firebase deploy`
* (e.g. PR-preview deploy actions). Both `glob({ follow: true })` and
* `glob({ follow: false })` would return symlink-to-file entries in
* the result (`follow` only controls whether symlinked *directories*
* are descended into); the explicit `lstatSync` filter below drops
* symlinks of either kind.
*/
export function listFiles(cwd: string, ignore: string[] = []): string[] {
return sync("**/*", {
const matched = sync("**/*", {
cwd,
dot: true,
follow: true,
follow: false,
ignore: ["**/firebase-debug.log", "**/firebase-debug.*.log", ".firebase/*"].concat(ignore),
nodir: true,
posix: true,
});
const out: string[] = [];
for (const rel of matched) {
let stats;
try {
// `lstat` does NOT follow symlinks, so we can detect them and skip.
stats = lstatSync(join(cwd, rel));
} catch {
// Stat error: skip the entry rather than risk uploading something
// we can't classify.
continue;
}
if (stats.isSymbolicLink()) {
logger.debug(
`[hosting] dropping symlink \`${rel}\` from upload list ` +
`(security: prevents symlink-following from exposing files outside the source tree)`,
);
continue;
}
out.push(rel);
}
return out;
}