[AI Improvement] [Task] Block real outbound network in unit tests with nock.disableNetConnect() in mocha-bootstrap.js

[joehan]

Resolves Buganizer b/566321794 (Parent Goal: b/566303602)

Proposed Improvement

• Configures nock.disableNetConnect() in src/test/helpers/mocha-bootstrap.js to fail fast with NetConnectNotAllowedError whenever a unit test initiates un-mocked outbound HTTPS requests to Google APIs (e.g. firebase.googleapis.com, compute.googleapis.com, cloudresourcemanager.googleapis.com).
• Allows loopback/local emulator traffic using nock.enableNetConnect(/^(localhost|127\.0\.0\.1|\[::1\]|::1)(:\d+)?$/).
• Eliminates the primary cause of flaky 2000ms timeouts and ECONNRESET retry hangs in unit test CI jobs.

Verification

• Full test suite: npm run mocha:fast passes all 5,566 unit tests (5566 passing, 11 pending, 0 failing).
• Full repo lint: npm run lint:quiet passes cleanly across the entire codebase.
• Project build: npm run build compiles cleanly.

[gemini-code-assist]

Code Review

This pull request disables external network connections globally in the Mocha bootstrap helper using nock.disableNetConnect() to prevent unit tests from reaching real Google APIs. A review comment suggests also re-applying these restrictions inside the global cleanup() function to prevent state leakage, as nock.cleanAll() does not reset the net-connect configuration if individual tests explicitly enable it.

[ajperel]

⚠️ Disclaimer: This draft review was generated by an experimental AI review agent. Please verify all findings before acting on them.

Code Review Summary: firebase/firebase-tools #11178

🟢 Strengths & LGTM Aspects

• Fail-Fast Network Isolation: Disabling external outbound HTTP/HTTPS calls globally via nock.disableNetConnect() provides immediate, deterministic failure (NetConnectNotAllowedError) instead of waiting for 2000ms timeouts or flaky ECONNRESET retry loops when unit tests fail to mock a Google API.
• Localhost & Emulator Traffic Allowed: The regex filter correctly preserves loopback traffic (localhost, 127.0.0.1, [::1], ::1), ensuring in-process or local emulator servers continue to work seamlessly.
• Integration Test Exclusion: Correctly distinguishes integration test suites running out of scripts/ and dev/scripts/ that legitimately require real outbound network connectivity.
• Minimal, Surgical Footprint: The change is clean, self-contained within src/test/helpers/mocha-bootstrap.js, and introduces zero overhead or new dependencies to production CLI commands.
• Changelog Governance: Appropriately omits a CHANGELOG.md entry as this is an internal test infrastructure enhancement.

🔴 Overview of Findings

• Blocking [Test Isolation & State Leakage]: nock.cleanAll() in cleanup() does not reset nock's net-connect state. 15 existing test suites in the repository explicitly call nock.enableNetConnect() in their after() hooks, which leaves outbound network access permanently open for subsequent suites unless re-asserted in Mocha lifecycle hooks (beforeEach / afterEach).

💡 Potential Enhancements

• Preventing Unscoped enableNetConnect(): Complementing the beforeEach reset by guarding against unscoped calls at runtime in mocha-bootstrap.js (throwing or clamping to loopback), updating src/test/helpers/nock.ts, and adding an ESLint no-restricted-syntax rule to enforce scoping statically.

🟡 Nits & Non-Blocking Suggestions

• Loopback Constant: Extract /^(localhost|127\.0\.0\.1|\[::1\]|::1)(:\d+)?$/ to a named top-level constant.
• Environment Variable Escape Hatch: Check process.env.FIREBASE_ALLOW_NET_CONNECT === "true" for ad-hoc debugging in custom IDE runners.

[ajperel]

🔴 [Test Isolation & State Leakage] Re-assert net-connect restriction in Mocha hooks (beforeEach / afterEach)

Rationale:
nock.cleanAll() in cleanup() (afterEach) removes active HTTP interceptors, but does not reset nock's net-connect configuration. If any test or suite explicitly calls nock.enableNetConnect(), that unrestricted state persists across all subsequent tests in the same Mocha worker process.

Furthermore, 15 existing test suites in the repository (e.g., src/accountImporter.spec.ts, src/gcp/cloudfunctions.spec.ts, src/management/apps.spec.ts, src/ensureApiEnabled.spec.ts, etc.) contain:

after(() => {
  nock.enableNetConnect();
});

In Mocha, a suite's after() hook executes after the afterEach (cleanup()) hook of the suite's final test. If net-connect is only restricted once at bootstrap or only inside afterEach, any suite running after one of these files will execute its before() and initial test case with external network access wide open.

Suggested Fix:
Encapsulate the policy enforcement in a helper and re-apply it both during initial load and in beforeEach (as well as cleanup()):

const LOOPBACK_REGEXP = /^(localhost|127\.0\.0\.1|\[::1\]|::1)(:\d+)?$/;

function enforceNetConnectPolicy() {
  if (!isIntegrationTest) {
    nock.disableNetConnect();
    nock.enableNetConnect(LOOPBACK_REGEXP);
  }
}

enforceNetConnectPolicy();

And in exports.mochaHooks:

exports.mochaHooks = {
  beforeEach() {
    enforceNetConnectPolicy();
    suiteFakes = new Set(typeof sinon.getFakes === "function" ? sinon.getFakes() : []);
  },
  afterEach: cleanup,
};

[ajperel]

💡 [Enhancement] Guard against unscoped enableNetConnect() calls

Rationale:
Re-asserting the restriction in beforeEach protects across test boundaries. However, tests could still call nock.enableNetConnect() without arguments inside a test body, inadvertently opening outbound network access during execution.

Suggested Approaches:

1. Runtime Monkey-Patch in mocha-bootstrap.js:
   Wrap nock.enableNetConnect so that any unscoped invocation either throws an explicit error (fail-fast) or automatically clamps to the loopback regex:

   const origEnableNetConnect = nock.enableNetConnect.bind(nock);
   
   if (!isIntegrationTest) {
     nock.enableNetConnect = function (matcher) {
       const isUnscoped =
         !matcher ||
         matcher === ".*" ||
         matcher === "*" ||
         (matcher instanceof RegExp && (matcher.source === ".*" || matcher.source === "^.*$"));
   
       if (isUnscoped) {
         // Option A: Fail fast
         throw new Error(
           "Calling unscoped nock.enableNetConnect() in unit tests is prohibited. " +
           "Specify an explicit host pattern or mock the network request."
         );
         // Option B: Silently clamp to loopback
         // return origEnableNetConnect(LOOPBACK_REGEXP);
       }
       return origEnableNetConnect(matcher);
     };
   }

2. Guard the Undici Mock Wrapper (src/test/helpers/nock.ts):
   The 15 existing spec files calling nock.enableNetConnect() actually import src/test/helpers/nock.ts (which delegates to Undici's mockAgent.enableNetConnect()). Guarding enableNetConnect(matcher?) there prevents leaks through the custom fetch mocking layer as well.

3. Static Prevention via ESLint:
   Add a no-restricted-syntax rule to .eslintrc.js to catch zero-argument nock.enableNetConnect() calls at lint time before tests run:

   {
     selector: "CallExpression[callee.object.name='nock'][callee.property.name='enableNetConnect'][arguments.length=0]",
     message: "Calling unscoped nock.enableNetConnect() in unit tests is forbidden. Pass an explicit host pattern or mock the request."
   }

[ajperel]

It'd be a bigger change but you could try to clean up all the existing usages of enableNetConnect() and enforce better behavior in one PR if you want. But even the best effort options is a good start.

[joehan]

Thank you for the review and suggestions @ajperel!

In commit 1a4b10f:

1. Re-asserted Policy in Mocha Hooks: Defined enforceNetConnectPolicy() and re-asserted it both inside cleanup() (afterEach) and in exports.mochaHooks.beforeEach(). This guarantees that even when suites call nock.enableNetConnect() in after() or before() hooks, subsequent test cases and suites running within the same worker have their outbound network restrictions hermetically restored.
2. Guarded Unscoped nock.enableNetConnect(): Wrapped nock.enableNetConnect() for unit test runs to clamp any unscoped calls (no arguments, *, or universal .* regex) back to LOOPBACK_REGEXP, preventing accidental bypasses.
3. Loopback Regex Constant: Extracted the matcher to LOOPBACK_REGEXP = /^(localhost|127\.0\.0\.1|\[::1\]|::1)(:\d+)?$/.
4. Environment Variable Escape Hatch: Added process.env.FIREBASE_ALLOW_NET_CONNECT === "true" to allow ad-hoc debugging in IDEs without modifying code.

Verified locally with npm run build, npm run test:compile, npm run lint:quiet, and Mocha test runs across both affected and after()-hooked test suites.

[joehan]

Thanks @ajperel! Addressed all the points in commit df099ad:

1. Per-Test / Per-File Policy Enforcement:
   • Instead of checking process.argv once globally on module load, enforceNetConnectPolicy(ctx) is evaluated dynamically inside Mocha's root hooks (beforeEach and afterEach).
   • ctx.currentTest.file is resolved relative to process.cwd() (isIntegrationTestFile(currentFile)), checking if the active test belongs to scripts/ or dev/scripts/.
   • If a command mixes unit and integration test files, unit test cases have net-connect disabled with loopback enabled, while integration test cases have net-connect re-enabled for real network calls.
   • Using path.relative(process.cwd(), filePath) also eliminates the risk of false positives from the repository path itself containing "scripts".
2. Removed Environment Variable Escape Hatch:
   • Removed FIREBASE_ALLOW_NET_CONNECT.
3. Removed enableNetConnect Monkey-Patching:
   • Removed the runtime wrapping/clamping of nock.enableNetConnect(), keeping standard nock behavior unmodified without silent overrides.
   • Test isolation is hermetically maintained by re-asserting the policy in cleanup() (afterEach) and root beforeEach().

Verified locally with npm run build, npm run test:compile, npm run lint:quiet, and targeted Mocha test runs.

[ajperel]

Joe agent ... in another PR can you track the work to not call overly broad enableNetConnect() in other tests and making a good decision on if we should monkey patch and generally prevent it.

[joehan]

Thanks @ajperel!

In commit 17716a8:

1. Simplified if/else: Inverted the condition in enforceNetConnectPolicy so isIntegrationTestFile(currentFile) is handled directly in the if block, making the execution flow clear and readable.
2. Follow-up Work Tracked: Created Buganizer ticket b/568405137 ("[Task] Audit and replace over-broad nock.enableNetConnect() calls in unit test suites") under parent goal b/566303602 to audit and replace over-broad enableNetConnect() calls across the repository and decide on static ESLint enforcement vs runtime guards.