fix: stabilize Windows CI integration tests by joehan · Pull Request #10940 · firebase/firebase-tools

joehan · 2026-08-14T23:09:49Z

Description

Stabilizes Windows CI integration tests, addresses review feedback, and improves runner reliability.

Key Changes

Zip Slip Path Normalization & Protection (src/unzip.ts, src/unzip.spec.ts):
- Enforce parentWithSep trailing separator check in isChildDir to prevent prefix-matching sibling directories (e.g. /tmp/app vs /tmp/app-secret) from bypassing directory traversal protection.
- Case-normalize paths on win32 to prevent casing discrepancies between process.cwd() and archive entries from triggering false-positive Zip Slip errors.
- Added comprehensive unit tests for isChildDir.
Process Lifecycle & Timeout Management (scripts/integration-helpers/cli.ts):
- In CLIProcess.stop() on Windows, execute taskkill /pid ${p.pid} /T /F and await the process exit event with a 2-second timeout race fallback.
- Capture and clear the timeout timer with clearTimeout when process exit resolves first.
Port Cleanup Safety (scripts/hosting-tests/run.sh, scripts/triggers-end-to-end-tests/run.sh):
- Restrict socket matching to LISTENING state (lsof -sTCP:LISTEN on POSIX and $2 ~ port"$" && $4 == "LISTENING" on Windows) to prevent killing client test runner processes with active connections to emulator ports.
- Scope preview channel IDs and target filenames to runner OS and random run ID (${GITHUB_RUN_NUMBER}-${RUNNER_OS}-${RANDOM}) to prevent cross-OS test collisions in matrix CI runs.
Disable MOTD in CI (src/fetchMOTD.ts):
- Skip fetchMOTD() early when process.env.CI is set to avoid unnecessary network calls and potential timeouts during CI test execution.
CLI Commands in Integration Tests (scripts/hosting-tests/run.sh, scripts/storage-deploy-tests/run.sh):
- Use standard firebase target:apply <service> <target-name> <resource> --project <project> commands instead of writing .firebaserc directly.
Runner Stability & Windows Defender (.github/workflows/node-test.yml):
- Exclude workspace and npm directories from Windows Defender (Set-MpPreference -ExclusionPath @("${{ github.workspace }}", "$env:LocalAppData\npm-cache", "$env:AppData\npm")) with explanatory documentation.
- Removed npm@9.5 downgrade step on Node 24 Windows runners to eliminate Arborist deadlock during npm ci.
- Added --prefer-offline --no-audit across fixture dependency installations in test runners.
Client Integration Tests (scripts/client-integration-tests/tests.ts, run.sh):
- Add explicit this: Mocha.Suite annotations.
- Add this.timeout(15 * 1000) and this.retries(2) to apps:list and apps:sdkconfig suites.
- Run apps.list variants in parallel using Promise.all.
- Wrap .firebaserc deletion in deployHosting after hook with try/catch.
- Increase Mocha timeout to 30000ms in scripts/client-integration-tests/run.sh.

CI Validation Runs

All 6 Windows integration test suites (client-integration, emulator, hosting, triggers-end-to-end:inspect, functions-discover, storage-deploy) and all 12 Linux suites have been validated with 100% GREEN (53/53 passing checks):

Run	Commit	Status	Windows Integration Suites Duration
Latest Validation Run (Run 32197607736)	`b667bc42d`	53/53 PASSED (100%)	`client-integration` (2m43s), `emulator` (4m29s), `hosting` (7m11s), `triggers:inspect` (8m26s), `functions-discover` (6m45s), `storage-deploy` (6m38s)
Validation Run (Run 32084241549)	`fa6cdcd78`	50/50 PASSED (100%)	`client-integration` (3m50s), `emulator` (4m26s), `hosting` (7m9s), `triggers:inspect` (8m6s), `functions-discover` (8m21s), `storage-deploy` (11m54s)
Validation Run (Run 31884410611)	`b10a8181d`	50/50 PASSED (100%)	`client-integration` (2m39s), `emulator` (4m41s), `hosting` (6m55s), `triggers:inspect` (9m8s), `functions-discover` (7m24s), `storage-deploy` (6m53s)

gemini-code-assist

Code Review

This pull request introduces several test stability improvements, including Windows-specific process teardown using taskkill, increased timeouts, parallelized test execution, and skipping MOTD fetching in CI environments. Feedback on these changes highlights a potential race condition on Windows where the process teardown promise is resolved too early, implicit any type violations on the this parameter in test suites, and a redundant process.env.CI check before calling fetchMOTD().

gemini-code-assist · 2026-08-14T23:11:15Z

 });

-describe("apps:list", () => {
+describe("apps:list", function (this) {


In TypeScript, the special this parameter must have an explicit type annotation (e.g., this: Mocha.Suite). Leaving it as function (this) results in an implicit any type, which violates the repository style guide's rule against using any as an escape hatch.

describe("apps:list", function (this: Mocha.Suite) {

References

Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. ^(link)

Done! Added explicit type annotation.

gemini-code-assist · 2026-08-14T23:11:15Z

 });

-describe("apps:sdkconfig", () => {
+describe("apps:sdkconfig", function (this) {


In TypeScript, the special this parameter must have an explicit type annotation (e.g., this: Mocha.Suite). Leaving it as function (this) results in an implicit any type, which violates the repository style guide's rule against using any as an escape hatch.

describe("apps:sdkconfig", function (this: Mocha.Suite) {

References

Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. ^(link)

Done! Added explicit type annotation.

gemini-code-assist · 2026-08-14T23:11:15Z

+  if (!process.env.CI) {
+    fetchMOTD();
+  }


This process.env.CI check is redundant because fetchMOTD() itself already performs the exact same check and returns early if process.env.CI is set. We can simplify this by calling fetchMOTD() directly.

fetchMOTD();

joehan · 2026-08-14T23:18:15Z

/joe-review

…l, and fix debug log step

…gers e2e tests

…emulator/triggers tests

…ean-install cleanup

…sting tests

…arget

…ng-tests

…r in pnpm fixture

…ndows runners

…--no-audit to npm ci

…ock during npm ci

…ncies

… per run

…n Windows

…itivity in unzip

… commands

…ase use

ajperel · 2026-08-18T22:12:34Z

+      const lowerParent = resolvedParent.toLowerCase();
+      const lowerChild = resolvedChild.toLowerCase();
+      return (
+        (lowerChild.startsWith(lowerParent + path.sep) || lowerChild.startsWith(lowerParent)) &&


🔴 [Security] Zip Slip Vulnerability Bypass (Windows)

Rationale: The current logic (lowerChild.startsWith(lowerParent + path.sep) || lowerChild.startsWith(lowerParent)) will return true if the child path starts with the parent path, even if it is not a sub-directory but a sibling with a matching prefix (e.g., C:\foo-bar starts with C:\foo). This allows zip-slip traversal to sibling directories.

Suggested Fix:
Ensure lowerParent ends with a separator before checking startsWith:

const lowerParent = resolvedParent.toLowerCase(); const lowerChild = resolvedChild.toLowerCase(); const parentWithSep = lowerParent.endsWith(path.sep) ? lowerParent : lowerParent + path.sep; return lowerChild.startsWith(parentWithSep) && lowerChild !== lowerParent;

Ok, that seems valid, will fix.

ajperel · 2026-08-18T22:12:34Z

+        p.once("exit", () => resolve());
+      });
+
+      const timeoutPromise = new Promise<void>((resolve) => setTimeout(resolve, 2000));


🟡 Nit: Uncleared timeout

Rationale: The setTimeout is not cleared if the process exits before the 2-second timeout, which can keep the event loop active.

Suggested Fix:

let timeoutId: NodeJS.Timeout; const timeoutPromise = new Promise<void>((resolve) => { timeoutId = setTimeout(resolve, 2000); }); try { execSync(`taskkill /pid ${p.pid} /T /F`); } catch { // ignore if process already exited } return Promise.race([exitPromise, timeoutPromise]).then(() => { clearTimeout(timeoutId); this.process = undefined; });

ajperel · 2026-08-18T22:22:20Z

          - node-version: "24.18"
            script: "npm run test:functions-discover"
    steps:
+      - name: Disable Windows Defender for workspace


Windows Defender sounds like it could be important. Should you add a comment on why this is necessary / good to do?

Reasonable - will add a brief explanation here

ajperel · 2026-08-18T22:26:16Z

 echo "${DATE}" > "public/${TARGET_FILE}"
 echo "Initialized temp directory."

+function kill_port() {


(Also from AI)

These commands match both local and foreign ports. If the test runner (or any other process in the CI VM) has an active client connection to the emulator port, the client process's ephemeral port info will also match the grep or lsof query.

For example, if the emulator is on port 8085 and the test runner connects to it from local port 54321:

netstat will show a line with 127.0.0.1:54321 -> 127.0.0.1:8085 (ESTABLISHED).
Because :8085 appears in the foreign address, grep ":8085 " will match this line and extract the PID of the client (the test runner).
The script will then execute taskkill on the test runner itself, causing the CI job to crash or fail mysteriously.

Recommended Fix:
Restrict the search to only processes listening on the target port.

For POSIX (lsof): Filter by TCP:LISTEN state:

# Before local pids=$(lsof -t -i:"$PORT_NUM" 2>/dev/null || true) # After (Safer) local pids=$(lsof -t -sTCP:LISTEN -i:"$PORT_NUM" 2>/dev/null || true)

For Windows (netstat): Use awk to ensure the port matches the Local Address (2nd column) and the state is LISTENING.

# Before local pids=$(netstat -ano | grep ":$PORT_NUM " | awk '{print $5}' | sort -u || true) # After (Safer) local pids=$(netstat -ano | awk -v port=":$PORT_NUM" '$2 ~ port"$" && $4 == "LISTENING" {print $5}' | sort -u || true)

…, and port filtering

… prevent cross-OS test collision

fix: stabilize Windows CI integration tests

0b1634a

gemini-code-assist Bot reviewed Aug 14, 2026

View reviewed changes

joehan commented Aug 14, 2026

View reviewed changes

Comment thread src/bin/cli.ts Outdated

joehan requested a review from ajperel August 14, 2026 23:29

joehan added 18 commits August 14, 2026 23:32

fix: address PR review feedback and increase unzip test timeout

7fa34d1

fix: write firebaserc directly in integration tests, simplify taskkil…

b4442f0

…l, and fix debug log step

fix: robust background process cleanup on Windows in hosting and trig…

5bdb30d

…gers e2e tests

fix: normalize path and case handling in unzip and scale timeouts in …

acf0ca9

…emulator/triggers tests

fix: add non-interactive flag and release working directory before cl…

6293de4

…ean-install cleanup

fix: generate full targets in .firebaserc directly for storage and ho…

27038cd

…sting tests

fix: add --non-interactive and --force to hosting channel deploy by t…

941862c

…arget

fix: place positional channelId argument before option flags in hosti…

b24a1f8

…ng-tests

fix: use robust tarball resolution in clean-install and hoisted linke…

5a0eeea

…r in pnpm fixture

fix: exclude npm cache and global npm directories from Defender on Wi…

f15c5a4

…ndows runners

fix: pass --exit to mocha in emulator tests and add --prefer-offline …

57b2b7a

…--no-audit to npm ci

ci: validation run 2

26ac598

fix: remove npm@9.5 downgrade in integration-windows to prevent deadl…

a36d3c2

…ock during npm ci

ci: validation run 3

b951a23

fix: initialize .firebaserc earlier in storage deploy test temp dir

911c033

fix: pass --exit to mocha and add --prefer-offline to fixture depende…

7f69fe6

…ncies

fix: kill processes by port in hosting-tests and isolate channel name…

b10a818

… per run

fix(test): await process exit with timeout fallback during taskkill o…

4766182

…n Windows

joehan commented Aug 17, 2026

View reviewed changes

Comment thread scripts/hosting-tests/run.sh Outdated

Comment thread src/unzip.ts

joehan added 4 commits August 17, 2026 23:24

fix(test): use firebase target:apply and document windows case-insens…

2892c90

…itivity in unzip

fix(test): add missing EOM delimiter in storage deploy tests

a91dfc0

fix(test): add --alias default and explicit --project to target:apply…

fa15318

… commands

fix(test): use firebase target:apply --project directly without fireb…

fa6cdcd

…ase use

ajperel reviewed Aug 18, 2026

View reviewed changes

fix: address review feedback for zip slip path check, process cleanup…

1ab79a4

…, and port filtering

fix(test): use unique runner-scoped channel names and file targets to…

b667bc4

… prevent cross-OS test collision

Uh oh!

Conversation

joehan commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes

CI Validation Runs

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

joehan commented Aug 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joehan commented Aug 14, 2026 •

edited

Loading