fix(vscode): resolve false "Firebase CLI not installed" warnings - #10874
fix(vscode): resolve false "Firebase CLI not installed" warnings#10874joehan wants to merge 2 commits into
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
There was a problem hiding this comment.
Code Review
This pull request improves the Firebase CLI detection by checking the user-configured firebasePath and using more robust version parsing. However, the review feedback identifies a potential command injection vulnerability from running user-configured paths with shell: true. Additionally, the feedback notes that passing the path and arguments as a single string under shell: true will fail if the path contains spaces, and suggests checking if the path exists locally to spawn it directly instead.
yuchenshi
left a comment
There was a problem hiding this comment.
I believe the original intention for the check is to make sure typing firebase in a shell works to unblock running commands manually like firebase deploy. Yes, firebasePath being set alone is enough to make the VSCE work, but that's not the point.
I agree to run firebase with a shell, but I don't believe running firebasePath captures the original intention. If the warning sounds like the VSCE isn't working, then we should reword the warning.
(Besides, I'm a bit concerned that npx firebase-tools --version will take too long. Or worse still, npx firebase/firebase-tools will clone from git and build locally, likely every time. That's a huge waste of CPU + RAM + networking + disk space. Maybe a timeout works better if really want this check. Also, running --tool:setup-check is pointless if firebasePath isn't a single path to a file.)
|
Good points (particularly on not running npx at startup, even in a non blocking fashion). Will refactor this (which incidentally makes some of the nitty injection comments from GCA irrelvant) and send back your way |
### Description
Fixes a bug where the Firebase VSCode extension incorrectly warns users that the Firebase CLI is not installed (or not available on $PATH).
This false warning triggers because:
1. The extension startup version check in `checkCLIInstallation()` was hardcoded to spawn `"firebase"`, completely ignoring the user-configured `firebasePath` setting (which defaults to `"npx -y firebase-tools@latest"`).
2. It ran `spawnSync` without `{ shell: true }` on macOS/Linux, preventing resolution when the command wasn't on the process's GUI env PATH (even when it was globally installed).
3. The version validation used `semver.valid()`, which evaluated to `null` if the CLI printed any extra characters (like update notices or deprecation warnings) to stdout alongside the version number.
This fix:
1. Retrieves the user's `firebase.firebasePath` setting and checks the version of that command.
2. Spawns the check command using `{ shell: true }` so it runs through the shell and resolves local paths/NPX commands.
3. Uses `semver.coerce()` to extract the version, making it robust against dirty stdout (like update notices).
### Scenarios Tested
- Tested checking with default `"npx -y firebase-tools@latest"` command: successfully resolved to the npx version without global installation.
- Tested checking with global `"firebase"` command: successfully resolved the version.
- Verified version parser handles trailing newlines and CLI update notice strings correctly.
### Sample Commands
n/a
5a0e363 to
d07ed9b
Compare
Description
Fixes a bug where the Firebase VSCode extension incorrectly warns users that the Firebase CLI is not installed (or not available on $PATH).
This false warning triggers because:
checkCLIInstallation()was hardcoded to spawn"firebase", completely ignoring the user-configuredfirebasePathsetting (which defaults to"npx -y firebase-tools@latest").spawnSyncwithout{ shell: true }on macOS/Linux, preventing resolution when the command wasn't on the process's GUI env PATH (even when it was globally installed).semver.valid(), which evaluated tonullif the CLI printed any extra characters (like update notices or deprecation warnings) to stdout alongside the version number.This fix:
firebase.firebasePathsetting and checks the version of that command.{ shell: true }so it runs through the shell and resolves local paths/NPX commands.semver.coerce()to extract the version, making it robust against dirty stdout (like update notices).Scenarios Tested
"npx -y firebase-tools@latest"command: successfully resolved to the npx version without global installation."firebase"command: successfully resolved the version.Sample Commands
n/a