Skip to content

Commit 3b0459a

Browse files
sarg3ntsarg3nt
committed
feat: Overhaul GitHub workflows for CI/CD, security, and automation
- Added comprehensive audit and modernization of CI/CD workflows in the Gearbox monorepo. - Implemented multiple phases including cleanup, CI improvements, security hardening, release pipeline enhancements, and monthly automation. - Introduced new workflows for OpenSSF Scorecard and PR labeling. - Enhanced security by upgrading actions, adding rate limiting, and validating backup paths. - Improved user interface by escaping HTML in various JavaScript files to prevent XSS vulnerabilities. - Established a new rate limiter middleware to protect against brute force attacks on authentication endpoints.
1 parent 7a1647f commit 3b0459a

9 files changed

Lines changed: 437 additions & 33 deletions

File tree

docs/security-implementation-summary.md renamed to docs/reports/security-implementation-summary.md

File renamed without changes.

‎docs/reports/security-review-2026-02-01.md‎

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
# Gearbox Security Review Report
2+
3+
**Date:** 2026-02-01
4+
**Scope:** Full codebase (`gearbox/` and `gearbox-agent/`)
5+
**Method:** Static analysis against AI Code Security Anti-Patterns (breadth version)
6+
7+
---
8+
9+
## Executive Summary
10+
11+
The Gearbox codebase demonstrates **strong security practices** across both the dashboard and agent applications. No critical production vulnerabilities were found. The codebase uses proper parameterized queries, cryptographically secure random generation, bcrypt password hashing, and comprehensive authentication middleware. A small number of medium and low severity issues were identified, primarily around XSS in JavaScript client-side code and a SQL injection edge case in the backup function.
12+
13+
**Overall Security Posture: STRONG**
14+
15+
---
16+
17+
## Findings Summary
18+
19+
| # | Finding | Severity | CWE | Location |
20+
|---|---------|----------|-----|----------|
21+
| 1 | Unescaped user data in innerHTML (traffic filter label) | High | CWE-79 | `static/js/traffic/traffic-visualization.js:417,451` |
22+
| 2 | Backend data rendered without escaping (backup restore) | High | CWE-79 | `static/js/haproxy_config/editor.js:1400-1410` |
23+
| 3 | SQL injection in VACUUM INTO (backup path) | Medium | CWE-89 | `gearbox/internal/framework/database/backup.go:38` |
24+
| 4 | No rate limiting on dashboard login endpoint | Medium | CWE-307 | `gearbox/cmd/server/main.go` (login routes) |
25+
| 5 | TLS verification can be disabled via env var | Medium | CWE-295 | `gearbox/internal/framework/agent/client.go:70-78` |
26+
| 6 | Cookie Secure flag not set by default | Low | CWE-614 | `gearbox/internal/framework/auth/auth.go:46` |
27+
| 7 | CSP allows 'unsafe-inline' for scripts/styles | Low | CWE-79 | `gearbox/internal/framework/middleware/security_headers.go:60-76` |
28+
| 8 | Swagger docs publicly accessible on agent | Low | CWE-200 | `gearbox-agent/internal/api/server.go:83-85` |
29+
| 9 | Temp file created without explicit permissions | Low | CWE-377 | `gearbox-agent/internal/plugins/security/plugin.go:869` |
30+
| 10 | Error messages may expose internal details | Low | CWE-209 | Multiple locations |
31+
32+
---
33+
34+
## Detailed Findings
35+
36+
### 1. HIGH - XSS: Unescaped Filter Label in Traffic Visualization
37+
38+
**CWE-79 (Cross-Site Scripting)**
39+
40+
**File:** `gearbox/static/js/traffic/traffic-visualization.js`
41+
**Lines:** 417, 451
42+
43+
**Description:** The `activeFilter.label` value is interpolated directly into an innerHTML assignment without escaping. If the label contains user-controlled data (IP addresses, backend names), it could execute arbitrary JavaScript.
44+
45+
**Vulnerable code pattern:**
46+
47+
```javascript
48+
tbody.innerHTML = `<tr><td>No traffic matching filter: ${activeFilter?.label || ''}</td></tr>`;
49+
```
50+
51+
**Recommendation:** Use `escapeHtml()` (already defined elsewhere in the codebase) before interpolation:
52+
53+
```javascript
54+
tbody.innerHTML = `<tr><td>No traffic matching filter: ${escapeHtml(activeFilter?.label || '')}</td></tr>`;
55+
```
56+
57+
---
58+
59+
### 2. HIGH - XSS: Backend Data in Backup Restore UI
60+
61+
**CWE-79 (Cross-Site Scripting)**
62+
63+
**File:** `gearbox/static/js/haproxy_config/editor.js`
64+
**Lines:** 1400-1410
65+
66+
**Description:** Backend API response data (`b.reason`, `b.id`) is rendered via innerHTML without escaping. If the database contains malicious data, it could execute scripts. The `b.id` is also used in an inline `onclick` handler, which is vulnerable to attribute escape attacks.
67+
68+
**Recommendation:** Escape all dynamic values with `escapeHtml()` and use event listeners instead of inline onclick handlers.
69+
70+
---
71+
72+
### 3. MEDIUM - SQL Injection in Database Backup
73+
74+
**CWE-89 (SQL Injection)**
75+
76+
**File:** `gearbox/internal/framework/database/backup.go`
77+
**Line:** 38
78+
79+
**Description:** The backup path is constructed and interpolated directly into a SQL statement via `fmt.Sprintf`:
80+
81+
```go
82+
_, err := d.db.Exec(fmt.Sprintf("VACUUM INTO '%s'", backupPath))
83+
```
84+
85+
SQLite's `VACUUM INTO` does not support parameterized queries, but the backup path should be validated to contain only safe filesystem characters before use.
86+
87+
**Recommendation:** Add explicit path validation (alphanumeric, slashes, hyphens, underscores, dots only) before constructing the query. Reject any path containing single quotes.
88+
89+
---
90+
91+
### 4. MEDIUM - No Rate Limiting on Dashboard Login
92+
93+
**CWE-307 (Improper Restriction of Excessive Authentication Attempts)**
94+
95+
**Description:** The dashboard login endpoint has account lockout (5 failed attempts, 15-minute lockout) but no per-IP rate limiting. Distributed brute force attacks across multiple accounts could bypass the per-account lockout.
96+
97+
**Current mitigation:** Account lockout after 5 failures.
98+
99+
**Recommendation:** Add per-IP rate limiting to the login endpoint, similar to the token bucket algorithm already implemented in the agent (`gearbox-agent/internal/framework/middleware/ratelimit.go`).
100+
101+
---
102+
103+
### 5. MEDIUM - TLS Verification Bypass via Environment Variable
104+
105+
**CWE-295 (Improper Certificate Validation)**
106+
107+
**File:** `gearbox/internal/framework/agent/client.go`
108+
**Lines:** 70-78
109+
110+
**Description:** Setting `GEARBOX_INSECURE_TLS=true` disables TLS certificate verification. While this is intentional for development, it could be exploited if an attacker gains control of environment variables.
111+
112+
**Current mitigation:** Warning logged when enabled; `#nosec G402` annotation present.
113+
114+
**Recommendation:** Document the security implications clearly and consider restricting this to explicitly non-production environments.
115+
116+
---
117+
118+
### 6. LOW - Cookie Secure Flag Default
119+
120+
**CWE-614 (Sensitive Cookie in HTTPS Session Without 'Secure' Attribute)**
121+
122+
**File:** `gearbox/internal/framework/auth/auth.go`
123+
**Line:** 46
124+
125+
**Description:** The `Secure` cookie flag defaults to `false` and is only enabled when TLS is configured. If the application is deployed behind a TLS-terminating proxy without being configured for TLS, session cookies could be sent over HTTP.
126+
127+
**Current mitigation:** Warning logged when running without TLS.
128+
129+
---
130+
131+
### 7. LOW - CSP Allows unsafe-inline
132+
133+
**CWE-79 (Cross-Site Scripting)**
134+
135+
**File:** `gearbox/internal/framework/middleware/security_headers.go`
136+
**Lines:** 60-76
137+
138+
**Description:** The Content-Security-Policy allows `'unsafe-inline'` for both scripts and styles, which reduces XSS protection. This is documented as necessary for Tailwind CSS and inline event handlers.
139+
140+
**Recommendation:** Long-term, consider migrating to CSP nonces or hashes for inline scripts.
141+
142+
---
143+
144+
### 8-10. LOW - Minor Issues
145+
146+
- **Swagger docs public** on the agent could aid reconnaissance (`gearbox-agent/internal/api/server.go:83-85`)
147+
- **Temp file permissions** not explicitly set in security plugin (`gearbox-agent/internal/plugins/security/plugin.go:869`)
148+
- **Error messages** in some handlers use `%v` formatting which could expose internal paths
149+
150+
---
151+
152+
## Security Strengths
153+
154+
The codebase demonstrates many excellent security practices:
155+
156+
| Category | Implementation | Rating |
157+
|----------|---------------|--------|
158+
| **Secrets Management** | No hardcoded production secrets; env vars required; auto-generation with crypto/rand | Excellent |
159+
| **Password Security** | bcrypt cost 12; entropy validation (50+ bits); common password blacklist | Excellent |
160+
| **Session Management** | 128-bit crypto/rand tokens; server-side validation; DB-backed; proper cookie flags | Excellent |
161+
| **CSRF Protection** | 256-bit tokens; validated on all state-changing requests | Excellent |
162+
| **SQL Injection** | Parameterized queries throughout; whitelist validation for dynamic columns | Excellent |
163+
| **Command Injection** | exec.Command with separate args (no shell); input validation on IPs, packages, branches | Strong |
164+
| **Cryptography** | crypto/rand everywhere; TLS 1.2+ enforced; AES-256-GCM for API key encryption | Excellent |
165+
| **Authentication** | Bearer token with constant-time comparison; WebSocket token auth (60s TTL); passkey support | Excellent |
166+
| **Rate Limiting** | Token bucket algorithm on agent (50 req/s, burst 100); per-IP tracking | Strong |
167+
| **Input Validation** | SQL injection detection; email RFC 5322 validation; branch name whitelist regex | Strong |
168+
| **Security Headers** | CSP, X-Frame-Options DENY, X-Content-Type-Options, HSTS, Referrer-Policy | Strong |
169+
| **WebSocket Security** | Origin validation; short-lived single-use tokens; configurable allowed origins | Excellent |
170+
| **Audit Logging** | Login/logout/password changes logged with IP and user-agent | Strong |
171+
| **Config Redaction** | Automatic redaction of passwords, tokens, and secrets in HAProxy configs | Excellent |
172+
| **File Permissions** | API key files stored with 0600; proper backup directory validation | Strong |
173+
174+
---
175+
176+
## Recommendations by Priority
177+
178+
### Immediate
179+
180+
1. Escape `activeFilter.label` in `traffic-visualization.js` (lines 417, 451)
181+
2. Escape backup data in `haproxy_config/editor.js` (lines 1400-1410)
182+
183+
### High Priority
184+
185+
3. Add per-IP rate limiting to dashboard login endpoint
186+
4. Add path validation before `VACUUM INTO` in `backup.go`
187+
188+
### Medium Priority
189+
190+
5. Add input validation for log source fields (Unit, FilePath, Priority) in `streamer.go`
191+
6. Add domain name validation before certbot/acme.sh commands in `collector.go`
192+
7. Consider DOMPurify for complex HTML rendering in JavaScript
193+
194+
### Low Priority
195+
196+
8. Document `GEARBOX_INSECURE_TLS` security implications
197+
9. Protect Swagger docs in production agent deployments
198+
10. Set explicit permissions (0600) on temp files
199+
11. Review error messages for information disclosure
200+
12. Long-term: migrate from CSP `unsafe-inline` to nonces
201+
202+
---
203+
204+
## Domains Reviewed
205+
206+
| Security Domain | Issues Found | Notes |
207+
|-----------------|-------------|-------|
208+
| 1. Secrets and Credentials | 0 | No hardcoded secrets; proper encryption at rest |
209+
| 2. Injection (SQL/Cmd/LDAP) | 1 Medium | VACUUM INTO path; command execution is safe |
210+
| 3. Cross-Site Scripting | 2 High, 1 Low | innerHTML without escaping in 2 JS files |
211+
| 4. Authentication and Sessions | 1 Medium, 1 Low | Missing login rate limiting; cookie Secure flag |
212+
| 5. Cryptographic Failures | 1 Medium | TLS skip verify option (documented/intentional) |
213+
| 6. Input Validation | 0 | Comprehensive validation framework |
214+
| 7. Configuration and Deployment | 1 Low, 1 Low | Swagger exposure; error message detail |
215+
| 8. Dependency and Supply Chain | Not Scanned | Recommend separate `go mod audit` |
216+
| 9. API Security | 0 | Proper auth middleware on all protected routes |
217+
| 10. File Handling | 1 Low | Temp file permissions |

docs/workflow-updates.md renamed to docs/research/workflow-updates.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
# GitHub Workflows Overhaul - Task List
22

3+
**Status: COMPLETE** - All phases implemented and verified as of February 2026. Only the optional stale issue/PR workflow (7.3) was intentionally skipped.
4+
35
Comprehensive audit and modernization of CI/CD workflows for the Gearbox monorepo.
46

57
## Table of Contents

‎gearbox-agent/internal/plugins/security/plugin.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -872,6 +872,12 @@ func (p *Plugin) validateConfig(content string) (string, error) {
872872
}
873873
defer os.Remove(tmpFile.Name())
874874

875+
// Ensure restrictive permissions on temp file (owner read/write only)
876+
if err := os.Chmod(tmpFile.Name(), 0600); err != nil {
877+
tmpFile.Close()
878+
return "", fmt.Errorf("failed to set temp file permissions: %w", err)
879+
}
880+
875881
if _, err := tmpFile.WriteString(content); err != nil {
876882
tmpFile.Close()
877883
return "", fmt.Errorf("failed to write temp file: %w", err)

‎gearbox/cmd/server/main.go‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -498,24 +498,32 @@ func main() {
498498
_, _ = w.Write([]byte("# Prometheus metrics will be added here\n"))
499499
})
500500

501-
// Public routes - authentication
502-
r.Get("/login", h.LoginPage)
503-
r.Post("/login", h.LoginPost)
504-
505-
// Public routes - account request
506-
r.Get("/request-account", h.RequestAccountPage)
507-
r.Post("/request-account", h.RequestAccountPost)
508-
r.Get("/request-account/success", h.RequestAccountSuccessPage)
509-
510-
// Public routes - password reset
511-
r.Get("/forgot-password", h.ForgotPasswordPage)
512-
r.Post("/forgot-password", h.ForgotPasswordPost)
513-
r.Get("/reset-password", h.ResetPasswordPage)
514-
r.Post("/reset-password", h.ResetPasswordPost)
515-
516-
// Public routes - passkey authentication
517-
r.Get("/api/passkey/login/begin", h.PasskeyLoginBegin)
518-
r.Post("/api/passkey/login/finish", h.PasskeyLoginFinish)
501+
// Rate limiter for authentication endpoints (5 req/sec, burst of 10 per IP)
502+
// Protects against brute force and credential stuffing attacks
503+
authRateLimiter := gbmiddleware.NewRateLimiter(5, 10, logger)
504+
defer authRateLimiter.Close()
505+
506+
// Public routes - authentication (rate limited)
507+
r.Group(func(r chi.Router) {
508+
r.Use(gbmiddleware.RateLimitMiddleware(authRateLimiter))
509+
r.Get("/login", h.LoginPage)
510+
r.Post("/login", h.LoginPost)
511+
512+
// Account request
513+
r.Get("/request-account", h.RequestAccountPage)
514+
r.Post("/request-account", h.RequestAccountPost)
515+
r.Get("/request-account/success", h.RequestAccountSuccessPage)
516+
517+
// Password reset
518+
r.Get("/forgot-password", h.ForgotPasswordPage)
519+
r.Post("/forgot-password", h.ForgotPasswordPost)
520+
r.Get("/reset-password", h.ResetPasswordPage)
521+
r.Post("/reset-password", h.ResetPasswordPost)
522+
523+
// Passkey authentication
524+
r.Get("/api/passkey/login/begin", h.PasskeyLoginBegin)
525+
r.Post("/api/passkey/login/finish", h.PasskeyLoginFinish)
526+
})
519527

520528
// SSE endpoint - needs auth but NO timeout (long-lived connections)
521529
r.Group(func(r chi.Router) {

‎gearbox/internal/framework/database/backup.go‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"io"
66
"os"
77
"path/filepath"
8+
"regexp"
9+
"strings"
810
"time"
911
)
1012

@@ -33,9 +35,15 @@ func (d *DB) CreateBackup(backupDir string) (*BackupInfo, error) {
3335
d.mu.RLock()
3436
defer d.mu.RUnlock()
3537

38+
// Validate backup path to prevent SQL injection via VACUUM INTO
39+
// (VACUUM INTO does not support parameterized queries)
40+
if err := validateBackupPath(backupPath); err != nil {
41+
return nil, fmt.Errorf("invalid backup path: %w", err)
42+
}
43+
3644
// Execute VACUUM INTO to create a compact backup
3745
// This is safer than file copy and creates a clean, defragmented backup
38-
_, err := d.db.Exec(fmt.Sprintf("VACUUM INTO '%s'", backupPath))
46+
_, err := d.db.Exec(fmt.Sprintf("VACUUM INTO '%s'", backupPath)) //#nosec G201 -- backupPath validated by validateBackupPath
3947
if err != nil {
4048
return nil, fmt.Errorf("failed to create backup: %w", err)
4149
}
@@ -151,6 +159,21 @@ func DeleteBackup(backupPath string) error {
151159
return nil
152160
}
153161

162+
// validateBackupPath ensures the backup path is safe to use in a VACUUM INTO statement.
163+
// Since VACUUM INTO does not support parameterized queries, we must validate the path
164+
// to prevent SQL injection via crafted filenames or directory names.
165+
var validBackupPathRe = regexp.MustCompile(`^[a-zA-Z0-9/_.\-]+$`)
166+
167+
func validateBackupPath(path string) error {
168+
if strings.Contains(path, "'") {
169+
return fmt.Errorf("path must not contain single quotes")
170+
}
171+
if !validBackupPathRe.MatchString(path) {
172+
return fmt.Errorf("path contains invalid characters")
173+
}
174+
return nil
175+
}
176+
154177
// copyFile copies a file from src to dst.
155178
func copyFile(src, dst string) error {
156179
sourceFile, err := os.Open(src) //#nosec G304 -- paths are internal database/backup paths, not user-controlled

0 commit comments

Comments
 (0)