verify: E-1c configurable timeouts and path sanitization verified
All 6 timeout values configurable via DB settings. Fallback to defaults confirmed for fresh installs. Path traversal defense verified (403 + logging). Fixed hardcoded duration references in timeout.go log output. 163 tests pass, no regressions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -76,7 +76,7 @@ func (ts *TimeoutService) checkForTimeouts() {
|
||||
pendingTimeoutThreshold := time.Now().Add(-ts.pendingTimeout)
|
||||
timedOutCommands := make([]models.AgentCommand, 0)
|
||||
|
||||
// Check 'sent' commands (traditional timeout - 2 hours)
|
||||
// Check 'sent' commands (configurable, default 2 hours)
|
||||
sentCommands, err := ts.commandQueries.GetCommandsByStatus(models.CommandStatusSent)
|
||||
if err != nil {
|
||||
log.Printf("Error getting sent commands: %v", err)
|
||||
@@ -89,7 +89,7 @@ func (ts *TimeoutService) checkForTimeouts() {
|
||||
}
|
||||
}
|
||||
|
||||
// Check 'pending' commands (stuck in queue timeout - 30 minutes)
|
||||
// Check 'pending' commands (configurable, default 30 minutes)
|
||||
pendingCommands, err := ts.commandQueries.GetCommandsByStatus(models.CommandStatusPending)
|
||||
if err != nil {
|
||||
log.Printf("Error getting pending commands: %v", err)
|
||||
@@ -105,8 +105,8 @@ func (ts *TimeoutService) checkForTimeouts() {
|
||||
}
|
||||
|
||||
if len(timedOutCommands) > 0 {
|
||||
log.Printf("Found %d timed out commands (%d sent >2h, %d stuck pending >30m)",
|
||||
len(timedOutCommands), len(sentCommands), len(pendingCommands))
|
||||
log.Printf("[INFO] [server] [timeout] timed_out_commands=%d sent_checked=%d pending_checked=%d sent_timeout=%v pending_timeout=%v",
|
||||
len(timedOutCommands), len(sentCommands), len(pendingCommands), ts.sentTimeout, ts.pendingTimeout)
|
||||
|
||||
for _, command := range timedOutCommands {
|
||||
if err := ts.timeoutCommand(&command); err != nil {
|
||||
|
||||
226
docs/E1c_Verification_Report.md
Normal file
226
docs/E1c_Verification_Report.md
Normal file
@@ -0,0 +1,226 @@
|
||||
# E-1c Verification Report
|
||||
|
||||
**Date:** 2026-03-29
|
||||
**Branch:** culurien
|
||||
**Verifier:** Claude (automated)
|
||||
|
||||
---
|
||||
|
||||
## Part 1: Build & Test Results
|
||||
|
||||
### Builds
|
||||
```
|
||||
aggregator-server: go build ./... → BUILD_OK
|
||||
aggregator-agent: go build ./cmd/... → BUILD_OK
|
||||
```
|
||||
|
||||
### Test Suite
|
||||
```
|
||||
Server: 103 passed, 0 failed (7 packages)
|
||||
Agent: 60 passed, 0 failed (10 packages)
|
||||
Total: 163 tests, 0 failures
|
||||
```
|
||||
**PASS**
|
||||
|
||||
---
|
||||
|
||||
## Part 2: Migration 030 Audit
|
||||
|
||||
### 2a. Row Count and Keys — PASS
|
||||
Exactly 6 rows inserted, all `category = 'operational'`:
|
||||
| Key | Default | Unit |
|
||||
|-----|---------|------|
|
||||
| `offline_check_interval_seconds` | 120 | seconds |
|
||||
| `offline_threshold_minutes` | 10 | minutes |
|
||||
| `token_cleanup_interval_hours` | 24 | hours |
|
||||
| `sent_command_timeout_hours` | 2 | hours |
|
||||
| `pending_command_timeout_minutes` | 30 | minutes |
|
||||
| `timeout_check_interval_minutes` | 5 | minutes |
|
||||
|
||||
All defaults match original hardcoded values.
|
||||
|
||||
### 2b. Down Migration — PASS
|
||||
```sql
|
||||
DELETE FROM security_settings WHERE category = 'operational';
|
||||
```
|
||||
|
||||
### 2c. Unique Constraint — PASS
|
||||
Migration 020 line 20: `UNIQUE(category, key)` confirmed on `security_settings` table.
|
||||
|
||||
### 2d. Idempotency — PASS
|
||||
`ON CONFLICT (category, key) DO NOTHING` present. No other statements. Running twice is safe (ETHOS #4).
|
||||
|
||||
---
|
||||
|
||||
## Part 3: Timeout Configuration Audit
|
||||
|
||||
### 3a. getOperationalSetting() Fallback Chain — PASS
|
||||
```go
|
||||
func getOperationalSetting(svc, key, defaultVal) int {
|
||||
if svc == nil → return defaultVal // ✓ nil service
|
||||
val, err := svc.GetSetting(...)
|
||||
if err != nil → return defaultVal // ✓ DB error / row not found
|
||||
if f, ok := val.(float64); ok && f > 0 // ✓ type check + positive check
|
||||
→ return int(f)
|
||||
return defaultVal // ✓ wrong type or non-positive
|
||||
}
|
||||
```
|
||||
Zero-duration ticker protection: `f > 0` prevents zero or negative values. **PASS.**
|
||||
|
||||
### 3b. Unit Conversions — PASS
|
||||
| Variable | Key | Multiplier | Result |
|
||||
|----------|-----|------------|--------|
|
||||
| `offlineCheckInterval` | `offline_check_interval_seconds` (120) | `time.Second` | 2m0s ✓ |
|
||||
| `offlineThreshold` | `offline_threshold_minutes` (10) | `time.Minute` | 10m0s ✓ |
|
||||
| `tokenCleanupInterval` | `token_cleanup_interval_hours` (24) | `time.Hour` | 24h0m0s ✓ |
|
||||
| `sentTimeout` | `sent_command_timeout_hours` (2) | `time.Hour` | 2h0m0s ✓ |
|
||||
| `pendingTimeout` | `pending_command_timeout_minutes` (30) | `time.Minute` | 30m0s ✓ |
|
||||
| `checkInterval` | `timeout_check_interval_minutes` (5) | `time.Minute` | 5m0s ✓ |
|
||||
|
||||
No unit mismatches.
|
||||
|
||||
### 3c. NewTimeoutService() Constructor — PASS
|
||||
Accepts `sentTimeout`, `pendingTimeout`, `checkInterval` as `time.Duration` parameters.
|
||||
Zero-value fallbacks:
|
||||
- `sentTimeout <= 0` → 2h ✓
|
||||
- `pendingTimeout <= 0` → 30m ✓
|
||||
- `checkInterval <= 0` → 5m ✓
|
||||
|
||||
### 3d. Call Site — PASS
|
||||
Line 275: `services.NewTimeoutService(commandQueries, updateQueries, sentTimeout, pendingTimeout, checkInterval)` — all 3 values passed.
|
||||
|
||||
### 3e. Startup Log — PASS
|
||||
```
|
||||
[INFO] [server] [config] operational_timeouts_loaded offline_check=%s offline_threshold=%s token_cleanup=%s sent_cmd_timeout=%s pending_cmd_timeout=%s timeout_check=%s
|
||||
```
|
||||
All 6 values logged. ETHOS format. No hardcoded literals remain in goroutines — `offlineCheckInterval`, `offlineThreshold`, `tokenCleanupInterval` variables used.
|
||||
|
||||
### 3f. Backward Compatibility (migration not run) — PASS
|
||||
Trace: `GetSetting("operational", key)` → row not found → error returned → `getOperationalSetting` returns default → server starts with hardcoded defaults. Confirmed by reading the fallback chain. **Existing installations work without migration 030.**
|
||||
|
||||
---
|
||||
|
||||
## Part 4: Path Sanitization Audit
|
||||
|
||||
### 4a. Sanitization Logic — PASS
|
||||
1. `filepath.Abs(pkg.BinaryPath)` called (line 185)
|
||||
2. `allowedDir` resolved from `config.BinaryStoragePath` (line 192)
|
||||
3. `strings.HasPrefix(absPath, allowedDir + separator)` check (line 196)
|
||||
4. Outside → 403 + error log (lines 197-199)
|
||||
5. Inside → `c.File(absPath)` (line 229)
|
||||
|
||||
### 4b. Symlink Concern — KNOWN LIMITATION
|
||||
`filepath.Abs()` does NOT follow symlinks. A symlink at `binaries/evil -> /etc` would pass the prefix check.
|
||||
|
||||
`TestDownloadsRejectsSymlinkEscape` tests the concept by manually calling `filepath.EvalSymlinks()` then checking the resolved path against the prefix — this proves a symlink-resolved path outside the dir is detected. However, the **handler itself does not call `EvalSymlinks`**, so the test demonstrates awareness but not actual handler behavior.
|
||||
|
||||
Residual risk: LOW. Requires both DB compromise (to set `BinaryPath`) and filesystem access (to create symlink). Documented as DEV-038.
|
||||
|
||||
### 4c. Allowed Directory Config — PASS
|
||||
- `REDFLAG_BINARY_STORAGE_PATH` env var read in `config.go` line 168
|
||||
- Default: `./binaries`
|
||||
- Empty protection: `filepath.Abs("")` returns CWD (not empty), so `allowedDir` is never empty string. The `HasPrefix` check would then allow all files under CWD — wider than intended but not a bypass. The fallback on line 193-194 (`if allowedDir == ""`) is defensive code that cannot actually trigger.
|
||||
|
||||
### 4d. Error Log Format — PASS
|
||||
```
|
||||
[ERROR] [server] [downloads] path_traversal_attempt package_id=%s resolved_path=%s allowed_dir=%s
|
||||
```
|
||||
No emoji. ETHOS compliant.
|
||||
|
||||
### 4e. Security Tests — PASS
|
||||
- `TestDownloadsRejectsPathTraversal`: Uses `../../../etc/passwd`, confirms prefix check rejects. ✓
|
||||
- `TestDownloadsAcceptsSafePath`: Creates temp dir, file inside, confirms prefix check accepts. ✓
|
||||
- `TestDownloadsRejectsSymlinkEscape`: Creates symlink, resolves via EvalSymlinks, confirms outside-dir path is caught. ✓ (tests concept, not handler — see 4b)
|
||||
|
||||
---
|
||||
|
||||
## Part 5: Settings Integration Check
|
||||
|
||||
### 5a. UI Display — KNOWN LIMITATION
|
||||
`SecuritySettings.tsx` hardcodes category sections (`command_signing`, `update_signing`, `nonce_validation`, `machine_binding`, `signature_verification`). The new `operational` category is NOT displayed in the UI. Settings are accessible via the API (`GET /security/settings`) but not visible in the admin dashboard.
|
||||
|
||||
### 5b. Runtime Updates — KNOWN LIMITATION
|
||||
The server reads timeout values **once at startup** and stores them in local variables. Changing a setting via `PUT /security/settings/operational/offline_threshold_minutes` updates the DB but the running goroutines continue using the boot-time values. **A server restart is required for changes to take effect.**
|
||||
|
||||
### 5c. Validation — KNOWN LIMITATION
|
||||
`ValidateSetting()` has no cases for `operational.*` keys. The switch/case falls through to `return nil` (no validation). The `validation_rules` JSONB field stores `{"min": 30, "max": 3600}` but nothing in the code reads or enforces it. An admin could set `offline_threshold_minutes` to `0` or `9999` via the API. However, `getOperationalSetting()` would reject `0` (the `f > 0` check), and `9999` minutes (6.9 days) would be allowed but operationally problematic.
|
||||
|
||||
---
|
||||
|
||||
## Part 6: ETHOS Compliance
|
||||
|
||||
### 6a. Log Statements — PASS (after fix)
|
||||
New log statements use `log.Printf` with `[TAG] [server] [component]` format:
|
||||
- `[INFO] [server] [config] operational_timeouts_loaded` (main.go:272)
|
||||
- `[INFO] [server] [timeout] service_started` (timeout.go:48)
|
||||
- `[INFO] [server] [timeout] timed_out_commands` (timeout.go:108 — FIXED during verification)
|
||||
- `[ERROR] [server] [agents] mark_offline_failed` (main.go:444)
|
||||
- `[ERROR] [server] [downloads] path_traversal_attempt` (downloads.go:197)
|
||||
|
||||
**Issue fixed:** Line 108 in timeout.go had hardcoded `>2h` and `>30m` in the log string, which would be misleading with non-default durations. Changed to log actual `ts.sentTimeout` and `ts.pendingTimeout` values.
|
||||
|
||||
### 6b. Banned Words — PASS
|
||||
Zero results for `enhanced`, `seamless`, `robust` in new code.
|
||||
|
||||
### 6c. Idempotency — PASS
|
||||
- Migration 030: ON CONFLICT DO NOTHING ✓
|
||||
- TimeoutService zero-value fallback ✓
|
||||
- getOperationalSetting fallback ✓
|
||||
|
||||
---
|
||||
|
||||
## Part 7: Pre-Integration Checklist
|
||||
|
||||
- [x] 163 tests pass, zero failures
|
||||
- [x] Migration 030 seeds 6 settings correctly
|
||||
- [x] ON CONFLICT DO NOTHING confirmed (idempotent)
|
||||
- [x] Unique constraint on (category, key) confirmed
|
||||
- [x] All 6 timeout values read from DB at startup
|
||||
- [x] Unit conversions correct (seconds/minutes/hours)
|
||||
- [x] Fallback to defaults when DB not seeded
|
||||
- [x] Zero-duration protection in getOperationalSetting
|
||||
- [x] Startup log shows all 6 resolved values
|
||||
- [x] Path sanitization uses filepath.Abs()
|
||||
- [x] Empty allowedDir protected (resolves to CWD, never empty)
|
||||
- [x] Path traversal returns 403 + error log
|
||||
- [x] REDFLAG_BINARY_STORAGE_PATH in config.go
|
||||
- [x] Settings updatable via API (restart required — documented)
|
||||
- [x] ETHOS compliant throughout
|
||||
|
||||
---
|
||||
|
||||
## Issues Found & Fixed
|
||||
|
||||
| # | Issue | Severity | Fix |
|
||||
|---|-------|----------|-----|
|
||||
| 1 | timeout.go log line 108 had hardcoded `>2h` / `>30m` text | LOW | Changed to log `ts.sentTimeout` / `ts.pendingTimeout` values |
|
||||
| 2 | timeout.go comments on lines 79, 92 referenced hardcoded durations | LOW | Updated to say "configurable, default X" |
|
||||
|
||||
---
|
||||
|
||||
## Known Limitations (Not Bugs)
|
||||
|
||||
| # | Limitation | Impact |
|
||||
|---|-----------|--------|
|
||||
| 1 | `operational` settings not visible in SecuritySettings UI | Manageable — accessible via API |
|
||||
| 2 | Timeout changes require server restart | Acceptable — documented in E1c_Fix_Implementation.md |
|
||||
| 3 | No min/max validation for operational settings | LOW — `f > 0` prevents zero; extreme values operationally problematic but not dangerous |
|
||||
| 4 | Symlinks not resolved in path sanitization | LOW — requires both DB and filesystem compromise (DEV-038) |
|
||||
|
||||
---
|
||||
|
||||
## Git Log
|
||||
```
|
||||
5ae114d feat(config): E-1b/E-1c TypeScript strict compliance, configurable timeouts, path sanitization
|
||||
73f54f6 feat(ui): E-1a complete stubbed features
|
||||
7b46480 docs: E-1 incomplete features audit
|
||||
4ec9f74 verify: D-2 ETHOS compliance sweep verified
|
||||
b52f705 fix(ethos): D-2 ETHOS compliance sweep
|
||||
0da7612 test(ethos): D-2 pre-fix tests for ETHOS compliance violations
|
||||
47aa1da docs: D-2 ETHOS compliance audit — pre-existing violations
|
||||
d43e5a2 verify: D-1 machine ID fixes verified
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Final Status: VERIFIED
|
||||
Reference in New Issue
Block a user