[joehan]

Description

Resolves Buganizer b/576834330
Parent Goal: b/565072516 - [Goal] Clean up and evaluate linter warnings across the codebase (Wave 2)

This PR addresses all 151 no-useless-escape violations across 25 files in the repository and removes the "no-useless-escape": "warn" override from .eslintrc.js, allowing eslint:recommended's default "error" level to apply cleanly.

In addition to removing the useless escape characters (such as \/ or \. inside regex character classes, or \"/\' in template literals):

• Intentional fix in src/emulator/auth/widget_ui.ts (L37): Corrected var isSamlProvider = !!providerId.match(/^saml\./); to var isSamlProvider = !!providerId.match(/^saml\\./);. In the template literal, \. was previously evaluated to ., resulting in /^saml./ in the emitted browser script. Double-escaping ensures the emitted regex correctly tests for literal saml.. Added regression spec src/emulator/auth/widget_ui.spec.ts to prevent template-literal backslash stripping regressions.
• Intentional fix in src/mcp/prompts/apptesting/run_test.ts (L117, L120, L124): Fixed mismatched quotes around `appId`, `projectNumber`, `releaseId`, and `release_test_name` where single quotes were mixed with backticks.
• Intentional fix in src/mcp/prompts/core/deploy.ts (L83, L85, L88): Added missing closing backticks on `firebase.json` (fixing \ escaped spaces), closed the inline code block at the end of L83 (}}`) to avoid an escaped newline line continuation swallowing the newline before item 4b, and removed a stray literal TAB (0x09) in firebase_create_project` at L88.
• CHANGELOG Entry: Documented the user-visible formatting fix for the firebase:deploy prompt.

Scenarios Tested

• npm run lint:quiet: Passed cleanly across the entire repository with 0 errors.
• ESLint report: Verified 0 no-useless-escape violations remain repo-wide.
• npm run build: MCP apps and TypeScript compile passed cleanly with 0 errors.
• npm run test:compile: Type-check passed cleanly with 0 errors.
• node_modules/.bin/mocha: Targeted unit test suites passed cleanly:
  • src/emulator/auth/widget_ui.spec.ts (1 passing)
  • src/dataconnect/names.spec.ts
  • src/firestore/util.spec.ts
  • src/extensions/extensionsHelper.spec.ts
  • src/deploy/functions/runtimes/discovery/v1alpha1.spec.ts
  • src/emulator/extensions/postinstall.spec.ts
  • src/apphosting/repo.spec.ts
  • src/apphosting/githubConnections.spec.ts
  • src/frameworks/next/utils.spec.ts
  • src/mcp/**/*.spec.ts (180 passing)
• Character-by-character diff verification confirmed only flagged backslashes were removed across all non-flagged files.

Sample Commands

N/A

[gemini-code-assist]

Code Review

This pull request removes unnecessary backslash escapes across multiple files after disabling the "no-useless-escape" ESLint rule. While most changes are cleanups, the modifications in src/frameworks/next/testing/app.ts incorrectly remove necessary escapes inside a JSON string within a template literal, resulting in invalid and unparseable JSON. These escapes must be restored to prevent runtime errors.

[gemini-code-assist]

The backslashes escaping the double quotes inside the query parameters of the CSS URL (e.g., "path":"src/app/layout.tsx") were incorrectly removed as 'useless escapes'. Since this JSON string is defined inside a JavaScript template literal, a single backslash \" evaluates to an unescaped double quote " in the resulting string. This unescaped double quote prematurely terminates the JSON string key, making the entire manifest invalid and unparseable JSON. Please restore the double backslashes (e.g., \\"path\\":\\"src/app/layout.tsx\\") to ensure the emitted string contains the required \" escape sequence.

[gemini-code-assist]

The backslashes escaping the double quotes inside the query parameters of the CSS URL (e.g., "path":"src/app/layout.tsx") were incorrectly removed as 'useless escapes'. Since this JSON string is defined inside a JavaScript template literal, a single backslash \" evaluates to an unescaped double quote " in the resulting string. This unescaped double quote prematurely terminates the JSON string key, making the entire manifest invalid and unparseable JSON. Please restore the double backslashes (e.g., \\"path\\":\\"src/app/layout.tsx\\") to ensure the emitted string contains the required \" escape sequence.

[joehan]

🤖 Adversarial AI review (review skill)

Requested by joehan. This PR's worker agent should address every comment in this review: fix it, or reply on the thread with the reason it doesn't apply.
Decision: approved_with_suggestions · 0 blocking · 5 suggestions · 4 inline comments

No blocking issues. I checked every changed literal and couldn't break the change. Equivalence: across all 25 files, base has exactly 151 no-useless-escape reports and head has 0. In 22 files the head is byte-identical to ESLint's own removeEscape suggestions applied to base. In every file the non-literal code is identical, and every string/template literal cooks to the same value. Every regex literal parses to the same regexpp AST (which checks character classes, - placement, [^[\]], and [\\/]). new RegExp template inputs (tests.ts, extensionsHelper.ts:100, functionsEmulator.ts:1951) are byte-identical. The 3 intended changes are correct and called out in the PR. The widget now emits /^saml\./, matching the backend. The deploy prompt's backticks are balanced and the line break before step 4b is restored. The run_test quotes are fixed. Lint: --print-config shows no-useless-escape going from warn to error. A full npm run lint:quiet is clean, and a probe edit with a useless escape fails lint with exit 1. Upstream main is still at the base, so merging origin/main gives an identical tree. None of the 51 open PRs I could scan would add new violations once this lands. tsc --noEmit and test:compile pass, and 20 targeted spec files give 529 passing tests. All required CI checks are green. CodeQL (not a required check) re-reports alerts #456 and #480, which are already open on main. Intentionally left as-is: the \. in the new RegExp strings in tests.ts:506-508 never escaped anything, and the PR is right to keep that. Angular 19's application builder emits main-<hash>.js, which only matches because . is a wildcard. Also unchanged: the | inside getPathSep's character class and the odd '"singleProjectMode"' false' quoting in auth/server.ts. Merge conflicts this PR causes (all trivial): #11341 deletes src/apphosting/repo.ts (modify/delete); #11345 adds a rule next to the deleted .eslintrc.js line; #10741 deletes run_test.ts. #11340 merges cleanly. There is no PII/SPII or prompt-injection surface in this diff. Suggestions: an optional CHANGELOG line, a stray TAB in deploy.ts:88, a widget regression test, and CodeQL triage notes.

Findings outside the diff

1. SUGGESTION (docs)

   Optional (maintainer's call): add a CHANGELOG line for the user-visible firebase:deploy MCP prompt fix. The learnings skill (L146) and the PR-review skill (L125-L128) both say user-facing bug fixes need an entry. firebase:deploy is a core prompt with no experiment gate. On the base revision, the }}\ line continuation at deploy.ts:83 swallowed the newline. The agent therefore received step 4b glued onto the end of step 4a, inside an unbalanced code span: ...{"backendId": "<backendId>"}} 4b. If the app does NOT require SSR.... I verified this by evaluating the cooked template at both revisions. Precedent from today: #11334 added Fixed an issue where the Firebase MCP server referred to ... for a one-word MCP text typo. This is not blocking because b/576834330 explicitly says "A CHANGELOG line is optional; follow maintainer feedback." The other two text changes don't need their own entry. The widget SAML change has no realistic user-visible effect, because valid SAML IDs are saml.* and the backend already used /^saml\./. The apptesting:run_test prompt only ships behind the opt-in mcpalpha experiment. Suggested line for CHANGELOG.md, using the past-tense, no-bracket-tag format:

   - Fixed formatting errors in the `firebase:deploy` MCP prompt that merged the App Hosting and Hosting setup steps.

   Rule: firebase-tools-agent-learnings/changelog-entry-formatting

[joehan]

SUGGESTION (security)

No change requested here. CodeQL's "Incomplete string escaping or encoding" alert on this line is a re-report, not something this PR introduced. The same alert is already open on main: code-scanning #456 (js/incomplete-sanitization, lifecycleHooks.ts:12, open since 2025-02-26). It shows as "new" only because the line text changed. The behavior is identical: /\"/g and /"/g parse to the same regexpp AST, and sample commands produce byte-identical escapedCommand at both revisions.

It also isn't a real security risk. The input is the developer's own predeploy/postdeploy hook from firebase.json, which is deliberately run through a shell (spawn(translatedCommand, [], { shell: true })), so no trust boundary is crossed. What remains is a correctness quirk: a hook containing \" gets mangled by the outer "..." wrapper (echo \"hi\" becomes echo \\"hi\\").

"Escape backslashes first" is the wrong fix. It would change how every existing hook that contains a backslash or \$ is parsed, and in different ways on POSIX sh and cmd.exe. The right fix is to stop building a shell string, e.g. spawn(nodeExecutable, [crossEnvShellPath, command], { ...childOptions, shell: false }). That should be a separate PR tested on Windows (#11233 also edits this file). CodeQL is not a required check, so a maintainer can dismiss the PR-scoped alert as pre-existing.

Rule: general/codeql-triage

[joehan]

Acknowledged. Thank you for the detailed context and CodeQL triage analysis! Leaving this line as-is in this PR.

[joehan]

SUGGESTION (reliability)

The fix is correct and intended. I evaluated the cooked SCRIPT at both revisions. The browser now receives /^saml\./ instead of /^saml./, which matches the backend's own check (operations.ts:2867 uses /^saml\./) and the documented saml.* provider-ID format.

Effect on provider IDs:

• Still treated as SAML: saml.okta, saml..
• No longer treated as SAML: samlfoo, saml-foo, saml_foo, samlp.x.
• Unaffected: saml, SAML.okta, oidc.*, and the built-in IDs.

Before, those IDs got an extra SAMLResponse param, which the backend parsed but ignored outside its saml. branch. So no valid configuration changes behavior.

Nothing tests WIDGET_UI, though, and this bug (a template literal silently eating a \) is easy to reintroduce. Optional: add a tiny spec. I tried this one in a scratch worktree: it passes at this head, fails against the base widget_ui.ts, and lints clean.

// src/emulator/auth/widget_ui.spec.ts
import { expect } from "chai";

import { WIDGET_UI } from "./widget_ui";

describe("WIDGET_UI", () => {
  it("should only treat provider IDs with a literal 'saml.' prefix as SAML providers", () => {
    // The script is built from a template literal, so the source needs `\\.` for the browser to receive `\.`.
    expect(WIDGET_UI).to.include("providerId.match(/^saml\\./)");
  });
});

Rule: firebase-tools-pr-review/testing-discipline

[joehan]

Added src/emulator/auth/widget_ui.spec.ts asserting that WIDGET_UI includes providerId.match(/^saml\\./) to prevent template-literal escape regressions.

[joehan]

SUGGESTION (security)

No change requested here. The "Regular expression injection" alert is also a re-report. It is open on main as code-scanning #480 (js/regex-injection, functionsEmulator.ts:1951, since 2026-03-18). The cooked template string is byte-identical at both revisions for any projectId/trigger_name. I checked inputs containing ., (, and (a+)+$: an invalid pattern throws the same way at base and head.

In practice it isn't exploitable. handleHttpsTrigger returns 404 at L1910 unless the trigger key is an existing entry in this.triggers: ${region}-${trigger_name}, or just trigger_name on the background route. Prototype keys like constructor stop at the record.enabled check with a 204. So only developer-defined function IDs reach this RegExp, and only on the local emulator. What remains is a robustness issue: regex metacharacters in the interpolated values (e.g. the . in a legacy google.com: project ID) are unescaped.

Proper follow-up: wrap both interpolations in lodash's escapeRegExp (lodash is already a direct dependency), or drop the regex and strip the known /${projectId}/${region}/${trigger} prefix from url.pathname with string operations. Keep that out of this lint-only PR, per b/576834330's scope boundaries, and dismiss the PR-scoped alert as pre-existing.

Rule: general/codeql-triage

[joehan]

Acknowledged. Keeping this regex replacement strictly to the useless escape removal and leaving further sanitization/prefix trimming for a dedicated follow-up per b/576834330's scope boundaries.

[joehan]

SUGGESTION (docs)

Optional: this PR fixes typos in this prompt, but a stray literal TAB (0x09) is still inside the tool name on this line. cat -A shows firebase_create_project^I, so the agent receives `firebase_create_project<TAB>`. The real tool is firebase_create_project. It is a raw TAB rather than an escape, so no-useless-escape (and the task's removeEscape script) can't catch it. It dates from #9025 (2025-08-25). The line is in this PR's hunk and is the same kind of fix as L83/L85. If you'd rather keep the PR strictly to the flagged sites listed in b/576834330, a follow-up works too.

Suggested change:

-  5b. If the user chooses to create a new project, use the \`firebase_create_project	\` tool. Then set the new project as the active project (the \`firebase_update_environment\` tool may be helpful).
+  5b. If the user chooses to create a new project, use the \`firebase_create_project\` tool. Then set the new project as the active project (the \`firebase_update_environment\` tool may be helpful).

Rule: general/prompt-text-correctness

[joehan]

Fixed! Removed the stray literal TAB (0x09) after firebase_create_project in src/mcp/prompts/core/deploy.ts:88.

[joehan]

Addressed review feedback in commit c8bab45:

• src/emulator/auth/widget_ui.spec.ts: Added regression test asserting WIDGET_UI includes providerId.match(/^saml\./).
• src/mcp/prompts/core/deploy.ts: Removed stray literal TAB (0x09) after firebase_create_project in L88.
• CHANGELOG.md: Added entry for the firebase:deploy MCP prompt fix.
• CodeQL alert triage: Noted alerts Changelog for v.3.11.0 #456 and Merge target branch to master #480 as pre-existing reports from main.
• Verification: Full-repo npm run lint:quiet (0 errors), npm run build, npm run test:compile, and mocha test suites all pass cleanly.