fix(security): security fixes and CI optimization#1074
Conversation
- Add detailed squash merge benefits (7 key advantages) - Include recommended gh pr create commands with markdown file usage - Add explicit merge instructions for CLI and Web UI - Expand back-sync section with mandatory warnings and step-by-step guide - Add alternative back-sync method using GitHub CLI - Include verification commands and troubleshooting steps - Apply updates to both AI instructions and actual PR template - Ensure consistency between Copilot guidance and user-facing template This update ensures all PRs display comprehensive squash merge and back-sync instructions, preventing common issues like 'ahead' status and merge conflicts.
Phase 1 implementation complete with all code tasks validated. Testing to be completed in develop branch.
….28.0 to 0.33.1 Automatically merged Dependabot PR after CI validation.
Automatically merged Dependabot PR after CI validation.
Automatically merged Dependabot PR after CI validation.
- Update Go version from 1.25.4 to 1.25.5 in Dockerfile.chrome-go - Fixes HIGH severity vulnerability in stdlib HostnameError.Error() - Prevents excessive resource consumption from malicious certificates - Resolves quadratic runtime issue in error string construction Fixes: CVE-2025-61729 Related: https://github.com/GrammaTonic/github-runner/security/code-scanning/5682
Back-sync after PR #1073 (CVE-2025-61729 security fix) was merged to main. This prevents develop from appearing ahead of main and ensures branches stay synchronized.
- Update NPM_VERSION from 11.6.2 to 11.6.4 in Chrome and Chrome-Go Dockerfiles - Fixes HIGH severity vulnerability in glob (npm dependency) - npm 11.6.4 depends on glob ^13.0.0 (vs vulnerable 11.0.3) - Resolves command injection vulnerability in glob's -c/--cmd option - Prevents arbitrary code execution via malicious filenames Vulnerability Details: - CVE ID: CVE-2025-64756 - Severity: HIGH - Affected Package: glob 11.0.3 (npm internal dependency) - Fixed Versions: glob 11.1.0, 10.5.0 (npm 11.6.4 uses glob 13.0.0) - Issue: Command injection via shell metacharacters in filenames - Link: https://avd.aquasec.com/nvd/cve-2025-64756 Fixes: https://github.com/GrammaTonic/github-runner/security/code-scanning/5665
- Change VALIDATE_ALL_CODEBASE from true to false (only validate changed files) - Disable SAVE_SUPER_LINTER_OUTPUT to reduce artifact size - Disable VALIDATE_MD to reduce processing overhead - Expand FILTER_REGEX_EXCLUDE to skip docs and plan directories This reduces the Super-Linter job size and prevents image size issues while maintaining validation of critical files (Dockerfiles, bash, YAML, JSON). Fixes: https://github.com/GrammaTonic/github-runner/actions/runs/19948737687/job/57204156543
- Replace super-linter/super-linter with individual focused actions - Use hadolint/hadolint-action for Dockerfile linting - Use ludeeus/action-shellcheck for shell script validation - Use ibiqlik/action-yamllint for YAML validation - Add .yamllint.yml configuration file Benefits: - Significantly smaller action images (no 8GB+ Super-Linter image) - Faster execution with parallel specialized linters - More granular control over linting rules - Reduced CI/CD resource consumption - Better caching and incremental builds Fixes: https://github.com/GrammaTonic/github-runner/actions/runs/19948737687/job/57204156543
- Add 15-minute timeout for container scans (10m for filesystem) - Filter to CRITICAL and HIGH severity only to reduce scan time - Skip unnecessary directories (test-results, logs, .git) - Prevents PROTOCOL_ERROR from layer extraction timeouts This resolves the stream ID protocol errors when scanning large Docker images (Chrome/Chrome-Go runners with browsers and dependencies). Fixes: stream error: stream ID 17; PROTOCOL_ERROR; received from peer
Summary of ChangesHello @GrammaTonic, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers crucial enhancements across security, CI/CD efficiency, and observability. It patches critical vulnerabilities in core dependencies, streamlines the continuous integration process for faster and more reliable builds, and lays the groundwork for robust monitoring of self-hosted runners. The changes aim to create a more secure, performant, and transparent development environment. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces significant changes, including security fixes by upgrading Go and npm versions, and major CI optimizations by replacing Super-Linter with more focused actions. A large part of the change is the addition of a comprehensive Prometheus metrics collection and monitoring feature, complete with detailed planning documents, technical spikes, and implementation scripts. While the overall direction is excellent, there are some key areas for improvement. There's a major contradiction between the technical spike recommending a shell-based metrics server and the inclusion of an unused Go-based exporter, which should be resolved. Additionally, some of the new documentation is confusing and contains conflicting information. I've also provided suggestions to improve the new Go exporter by using build-time variables and structured logging, and pointed out a few minor issues in the new documentation and prompt files.
cmd/metrics-exporter/main.go
Outdated
| @@ -0,0 +1,125 @@ | |||
| package main | |||
There was a problem hiding this comment.
This PR includes a very detailed technical spike document (plan/spike-metrics-collection-approach.md) that explicitly recommends against using a Go-based exporter in favor of a netcat shell script approach to minimize dependencies and complexity. However, this Go exporter has been added anyway. Furthermore, the entrypoint.sh script only uses the shell-based metrics scripts, making this Go application and its dependencies (go.mod, go.sum) appear to be dead code.
Please clarify the intention here. If the shell-based approach is the chosen one, this Go code should be removed to avoid confusion and unnecessary maintenance. If this Go exporter is intended to be used, the entrypoint scripts and the spike document need to be updated accordingly.
| @@ -0,0 +1,671 @@ | |||
| # Grafana Dashboard & Metrics Endpoint Feature | |||
There was a problem hiding this comment.
This feature specification document is quite confusing as it presents multiple, conflicting implementation strategies. For instance, it includes a full Go-based implementation example and later a separate shell-based netcat example. This makes it difficult to understand the actual intended architecture.
To improve clarity and avoid misleading future developers, this document should be revised to reflect only the final, chosen implementation that is being merged. The alternative approaches and exploratory code should be moved to the technical spike document (plan/spike-metrics-collection-approach.md) where they belong.
| @@ -0,0 +1,169 @@ | |||
| --- | |||
| description: 'Systematically research and validate technical spike documents through exhaustive investigation and controlled experimentation.' | |||
| tools: ['runCommands', 'runTasks', 'edit', 'runNotebooks', 'search', 'extensions', 'usages', 'vscodeAPI', 'think', 'problems', 'changes', 'testFailure', 'openSimpleBrowser', 'fetch', 'githubRepo', 'todos', 'Microsoft Docs', 'search'] | |||
There was a problem hiding this comment.
The search tool is listed twice in the tools array. This is redundant and should be cleaned up to avoid potential confusion or errors in the tool-loading mechanism.
| tools: ['runCommands', 'runTasks', 'edit', 'runNotebooks', 'search', 'extensions', 'usages', 'vscodeAPI', 'think', 'problems', 'changes', 'testFailure', 'openSimpleBrowser', 'fetch', 'githubRepo', 'todos', 'Microsoft Docs', 'search'] | |
| tools: ['runCommands', 'runTasks', 'edit', 'runNotebooks', 'search', 'extensions', 'usages', 'vscodeAPI', 'think', 'problems', 'changes', 'testFailure', 'openSimpleBrowser', 'fetch', 'githubRepo', 'todos', 'Microsoft Docs'] |
|
|
||
| ### Current Branch | ||
| ```bash | ||
| cd /Users/grammatonic/Git/github-runner |
There was a problem hiding this comment.
This cd command uses a hardcoded absolute path to a user's local machine (/Users/grammatonic/...). This is not portable and will fail for any other user or in any automated environment. Please replace this with a relative path or a placeholder variable like $PROJECT_ROOT to ensure the script is universally usable.
| cd /Users/grammatonic/Git/github-runner | |
| cd . |
| ``` | ||
|
|
||
| - **Technology Stack Selection**: Document choice rationale for each layer |
There was a problem hiding this comment.
There appears to be a copy-paste error here. The line ``` is likely unintended and breaks the markdown formatting. It seems to be a remnant from the line above. Please remove it to ensure the document renders correctly.
| ``` | |
| - **Technology Stack Selection**: Document choice rationale for each layer | |
| - **Technology Stack Selection**: Document choice rationale for each layer | |
| - **Integration Points**: Define clear boundaries and communication protocols | |
| - **Deployment Architecture**: Docker containerization strategy |
cmd/metrics-exporter/main.go
Outdated
| package main | ||
|
|
||
| import ( | ||
| "log" |
There was a problem hiding this comment.
For a production-grade service like a metrics exporter, it's better to use a structured logging library (like slog, which is standard in Go 1.21+, or zerolog/zap) instead of the standard log package. Structured logs are machine-readable (e.g., JSON), which makes them much easier to parse, query, and filter in log aggregation systems. This would also allow you to add valuable context to your logs, such as the runnerName or runnerType.
cmd/metrics-exporter/main.go
Outdated
| var ( | ||
| runnerName = getEnvOrDefault("RUNNER_NAME", "unknown") | ||
| runnerType = getEnvOrDefault("RUNNER_TYPE", "standard") | ||
| runnerVersion = "2.329.0" |
There was a problem hiding this comment.
The runnerVersion is hardcoded. This is not ideal for maintainability, as it requires a code change for every version update. A better practice is to inject this value at build time using Go's -ldflags.
For example, you could change the variable declaration to var runnerVersion string and then build your binary with a command like:
GO_VERSION=$(cat VERSION)
go build -ldflags="-X 'main.runnerVersion=$GO_VERSION'" ...This decouples the application code from the version string, making your build and release process more robust.
var runnerVersion = "2.329.0"| # TYPE github_runner_info gauge | ||
| github_runner_info{runner_name="$RUNNER_NAME",runner_type="$RUNNER_TYPE",version="$RUNNER_VERSION"} 1 | ||
| METRICS |
There was a problem hiding this comment.
There seems to be a formatting error here. The METRICS heredoc terminator is inside the bash code block, which is incorrect syntax and makes the example confusing. Please move it outside the code block.
| # TYPE github_runner_info gauge | |
| github_runner_info{runner_name="$RUNNER_NAME",runner_type="$RUNNER_TYPE",version="$RUNNER_VERSION"} 1 | |
| METRICS | |
| github_runner_info{runner_name="$RUNNER_NAME",runner_type="$RUNNER_TYPE",version="$RUNNER_VERSION"} 1 |
METRICS
- Remove cmd/metrics-exporter/main.go (experimental Prometheus metrics collector) - Remove go.mod and go.sum (no longer needed without Go code) - This feature was not production-ready and conflicted with documented netcat-based metrics approach - Reduces codebase complexity and maintenance burden Part of CI/CD optimization effort after CVE-2025-61729 and CVE-2025-64756 security fixes.
1 participant
📋 Pull Request Description
🔀 Merge Strategy
This repository uses SQUASH MERGE as the standard merge strategy.
Summary
This PR includes critical security fixes and CI/CD pipeline optimizations:
Type of Change
Related Issues
🔄 Changes Made
Files Modified
docker/Dockerfile.chrome-go- Go 1.25.4 → 1.25.5docker/Dockerfile.chrome- npm 11.6.2 → 11.6.4docker/Dockerfile.chrome-go- npm 11.6.2 → 11.6.4.github/workflows/ci-cd.yml- Super-Linter replacement + Trivy optimization.yamllint.yml- New YAML linting configurationKey Changes
Security Fixes (2)
CVE-2025-61729 (HIGH) - Go stdlib vulnerability
CVE-2025-64756 (HIGH) - npm glob command injection
CI/CD Optimizations (2)
Super-Linter replacement
hadolint/hadolint-actionfor Dockerfile lintingludeeus/action-shellcheckfor shell script validationibiqlik/action-yamllintfor YAML validationTrivy scan resilience
Vulnerability Details
CVE-2025-61729:
CVE-2025-64756:
🧪 Testing
Testing Performed
Test Coverage
🔒 Security Considerations
📚 Documentation
🚀 Deployment Notes
✅ Checklist
🤖 AI Review Request
/cc @copilot
Note for Reviewers:
This PR contains critical security fixes that should be merged promptly:
Commits included: