ci: Build and push browser test Docker images in upload_test_artifacts #9049
Conversation
Enable the browser test runners in CI by building and pushing a specialized Docker image when cobalt_browsertests is a target. This update: 1. Authenticates with the us-west1 Artifact Registry. 2. Colocates the browser test Dockerfile with its dependencies in the artifacts directory. 3. Builds and pushes the image to: us-west1-docker.pkg.dev/ytlr-image-hosting/integration-test-images/avvall-service-test 4. Uses a unique tag based on the run ID and platform for traceability. This ensures that the necessary environment is available for downstream on-device or on-host browser test execution. Bug: 473909877
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds functionality to build and push a Docker image for browser tests within the CI/CD pipeline. A critical vulnerability was identified where a service account access token could be exposed in GitHub Actions logs due to set -x tracing combined with printf ${ACCESS_TOKEN}. Additionally, there are areas for improvement regarding shell script safety, robustness of JSON parsing, and maintainability, including an unsafe use of printf, a brittle JSON check, and a hardcoded image name.
- Remove 'set -x' to prevent ACCESS_TOKEN leakage in CI logs. - Use 'printf %s' for safe token handling to avoid interpretation of format specifiers or backslashes. - Add '-s' to curl for silent execution.
Pipe curl output directly into docker login to avoid using an intermediate variable for the access token, further improving security and readability.
Replace grep with a robust jq filter to detect cobalt_browsertests in the targets JSON. This prevents false positives and handles fully qualified target names correctly.
Add docker_image_name as an optional input to the action to avoid hardcoding. This improves reusability for different test suites and services.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new capability to the upload_test_artifacts action to build and push a Docker image for browser tests. A high-severity command injection vulnerability has been identified in .github/actions/upload_test_artifacts/action.yaml due to direct interpolation of GitHub Action inputs and matrix variables into a shell script, which can lead to arbitrary command execution. This should be remediated by using environment variables. Additionally, the action contains hardcoded values that reduce reusability and a potential correctness issue in the browser test target detection logic.
Refine the jq filter to check for exact target matches (or exact suffixes) rather than substrings. This prevents false positives from similarly named targets while still correctly handling fully qualified names.
- Use environment variables for inputs and matrix values in shell scripts to prevent command injection vulnerabilities. - Promote registry region, project, and repository to action inputs to improve reusability. - Rename 'Login to us-west1 GAR' to 'Login to GAR' to reflect its parameterized nature.
- Update on_device_tests_gateway_client.py to accept --docker_tag and include it in test parameters. - Add docker_tag input to on_device_tests and internal_tests actions. - Reconstruct and pass the Docker image tag in main.yaml for on-device, E2E, and YTS tests. This allows the test lab to identify and pull the correct Docker image for each test session.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates Docker image building and pushing for browser tests into the CI workflow. However, it introduces a high-severity command injection vulnerability in the internal_tests and on_device_tests actions, as the docker_tag input is directly interpolated into shell scripts instead of being handled safely via environment variables. Additionally, there is a high-severity suggestion to improve authentication with Google Artifact Registry and a medium-severity suggestion to parameterize the Dockerfile path for enhanced action flexibility.
Ensure all inputs, including docker_tag, are passed via environment variables in shell scripts within composite actions. This prevents arbitrary command execution from maliciously crafted input values.
- Remove Docker build/push from upload_test_artifacts action (runs in container). - Add docker-browsertests-image job to main.yaml to run on host. - Download test artifacts from GCS using gsutil in the new job. - Update downstream test jobs to depend on docker-browsertests-image.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - name: Parse Test Target JSON | ||
| id: test-target-json | ||
| uses: ./.github/actions/test_targets_json | ||
| - name: Build and Push Browser Test Docker Image |
Check warning
Code scanning / Scorecard
Pinned-Dependencies Medium
- Add cobalt/testing/browser_tests/tools to CI_ESSENTIALS. - Update cp command to use the correct relative path for the Dockerfile.
1 participant
Enable the browser test runners in CI by building and pushing a specialized Docker image when cobalt_browsertests is a target.
This update:
This ensures that the necessary environment is available for downstream on-device or on-host browser test execution.
Bug: 483488213