Skip to content

Add a skill for addressing vulnerable dependencies in the Firebase Docker image#10321

Open
joehan wants to merge 2 commits intomainfrom
jh-openssl
Open

Add a skill for addressing vulnerable dependencies in the Firebase Docker image#10321
joehan wants to merge 2 commits intomainfrom
jh-openssl

Conversation

@joehan
Copy link
Copy Markdown
Member

@joehan joehan commented Apr 10, 2026

Description

Adding a skill for checking for and addressign vulnerabilities flagged in the Firebase docker image.

Also, updated the firebase-docker-image publish script to make it simpler to stage builds for testing and vulnerability checking.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new skill for resolving Docker vulnerabilities, updates the picomatch dependency, and parameterizes the Docker build process in cloudbuild.yaml and run.sh. The review feedback highlights a regression caused by hardcoding the target project ID in the build configuration and suggests improvements to the run.sh script for better path handling and parameterization.

@@ -1,6 +1,8 @@
## Script for testing Docker image creation without running a full release.
PROJECT_ID=$1
REPO_NAME=${2:-us}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Consider adding a parameter for the target project ID and using dirname "$0" to ensure the script can correctly locate its configuration files when called from the repository root, as suggested in the SKILL.md workflow.

Suggested change
REPO_NAME=${2:-us}
REPO_NAME=${2:-us}
TARGET_PROJECT_ID=${3:-$PROJECT_ID}
DIR=$(dirname "$0")

Comment on lines +7 to +8
submit \
--substitutions=_REPO_NAME=$REPO_NAME No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Specify the configuration file and build context explicitly to ensure the command works when run from the repository root. Also, pass the _TARGET_PROJECT_ID substitution to allow overriding the registry project (which now defaults to firebase-cli in cloudbuild.yaml).

Suggested change
submit \
--substitutions=_REPO_NAME=$REPO_NAME
submit \
--config "$DIR/cloudbuild.yaml" \
--substitutions=_REPO_NAME="$REPO_NAME",_TARGET_PROJECT_ID="$TARGET_PROJECT_ID" \
"$DIR"


substitutions:
_REPO_NAME: us
_TARGET_PROJECT_ID: firebase-cli
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Hardcoding firebase-cli as the default for _TARGET_PROJECT_ID is a regression from the previous behavior where the image was pushed to the project running the build. This makes the build configuration less reusable for developers testing in their own projects.

Run the build on `fir-tools-builds` and publish to the `staging` repository in `firebase-cli` to see the baseline vulnerabilities after the build's own updates.

```bash
./scripts/publish/firebase-docker-image/run.sh fir-tools-builds staging
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If run.sh is updated to default the target project to the project running the build (restoring previous behavior), this command should explicitly specify firebase-cli as the third argument to maintain the intended workflow of pushing to the staging registry.

Suggested change
./scripts/publish/firebase-docker-image/run.sh fir-tools-builds staging
./scripts/publish/firebase-docker-image/run.sh fir-tools-builds staging firebase-cli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants