jpetree331
5ae114df7a
E-1b: Fix 217 TypeScript strict errors to zero (tsc --noEmit clean). - Remove unused vars/imports, fix type mismatches, widen interfaces - TanStack Query v5 isLoading->isPending for mutations - No @ts-ignore or as any introduced E-1ab verification fixes: - Fix audit table name mismatch (security_setting_audit -> security_settings_audit) - Fix DockerContainer TS type (image_name->image, image_tag->tag) to match server - Add 501 for empty binary_path in downloads - Fix ETHOS log format in downloads error path E-1c: Configurable timeouts + path sanitization - Seed 6 operational timeout settings in DB (migration 030) - Wire server to read timeout values from DB at startup - Fallback to hardcoded defaults if DB settings missing - Fix binary_path traversal risk in downloads handler - Add BinaryStoragePath config (REDFLAG_BINARY_STORAGE_PATH) - Log resolved timeout values at startup 163 tests pass (103 server + 60 agent). No regressions. Vite build passes. TypeScript: 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
116 lines
4.7 KiB
Markdown
116 lines
4.7 KiB
Markdown
# E-1c Configurable Timeouts + Path Sanitization
|
|
|
|
**Date:** 2026-03-29
|
|
**Branch:** culurien
|
|
|
|
---
|
|
|
|
## Summary
|
|
|
|
Two items completed:
|
|
1. **F-E1-3**: 6 hardcoded operational timeout values made configurable via DB settings
|
|
2. **Security**: Binary path traversal defense-in-depth for downloads handler
|
|
|
|
## F-E1-3: Configurable Timeout Values
|
|
|
|
### The 6 Hardcoded Values (Before)
|
|
|
|
| Location | Value | Purpose |
|
|
|----------|-------|---------|
|
|
| main.go:429 | `2 * time.Minute` | Offline agent check interval |
|
|
| main.go:436 | `10 * time.Minute` | Offline threshold (mark agent offline) |
|
|
| main.go:445 | `24 * time.Hour` | Token cleanup interval |
|
|
| timeout.go:28 | `2 * time.Hour` | Sent command timeout |
|
|
| timeout.go:29 | `30 * time.Minute` | Pending command timeout |
|
|
| timeout.go:40 | `5 * time.Minute` | Timeout check interval |
|
|
|
|
### Migration 030: Seed Operational Settings
|
|
|
|
Created `030_add_operational_settings.up.sql` which inserts 6 rows into `security_settings` under category `operational`:
|
|
|
|
| Key | Default | Range | Description |
|
|
|-----|---------|-------|-------------|
|
|
| `offline_check_interval_seconds` | 120 | 30-3600 | How often to check for offline agents |
|
|
| `offline_threshold_minutes` | 10 | 2-60 | Minutes before agent marked offline |
|
|
| `token_cleanup_interval_hours` | 24 | 1-168 | Hours between token cleanup runs |
|
|
| `sent_command_timeout_hours` | 2 | 1-24 | Hours before sent commands time out |
|
|
| `pending_command_timeout_minutes` | 30 | 5-120 | Minutes before pending commands time out |
|
|
| `timeout_check_interval_minutes` | 5 | 1-30 | How often timeout service checks |
|
|
|
|
Migration uses `ON CONFLICT (category, key) DO NOTHING` for idempotency (ETHOS #4). Leverages existing `UNIQUE(category, key)` constraint from migration 020.
|
|
|
|
### Fallback Strategy
|
|
|
|
A helper function `getOperationalSetting()` reads from the DB with fallback:
|
|
1. If `SecuritySettingsService` is nil (DB not ready): return default
|
|
2. If `GetSetting()` errors: return default
|
|
3. If value is not a valid positive number: return default
|
|
4. Otherwise: return the DB value
|
|
|
|
This means the server starts correctly even if migration 030 hasn't run yet.
|
|
|
|
### TimeoutService Constructor Change (Option A)
|
|
|
|
Changed `NewTimeoutService` to accept 3 additional duration parameters:
|
|
|
|
```go
|
|
func NewTimeoutService(cq, uq, sentTimeout, pendingTimeout, checkInterval time.Duration)
|
|
```
|
|
|
|
Zero values are replaced with defaults (2h, 30m, 5m). The check interval is now a configurable field instead of a hardcoded `5 * time.Minute` in `Start()`.
|
|
|
|
### Startup Log
|
|
|
|
```
|
|
[INFO] [server] [config] operational_timeouts_loaded offline_check=2m0s offline_threshold=10m0s token_cleanup=24h0m0s sent_cmd_timeout=2h0m0s pending_cmd_timeout=30m0s timeout_check=5m0s
|
|
```
|
|
|
|
## Security: Binary Path Sanitization
|
|
|
|
### The Risk (Flagged in E-1ab Part 4b)
|
|
|
|
`pkg.BinaryPath` from the database was passed directly to `c.File()`. If the database were compromised, arbitrary file read was possible via path traversal.
|
|
|
|
### The Fix
|
|
|
|
Before serving, the handler now:
|
|
1. Resolves `BinaryPath` to an absolute path via `filepath.Abs()`
|
|
2. Resolves the allowed directory from `config.BinaryStoragePath` (env: `REDFLAG_BINARY_STORAGE_PATH`, default: `./binaries`)
|
|
3. Checks that the resolved path has the allowed directory as a prefix
|
|
4. If not: returns 403 and logs `[ERROR] [server] [downloads] path_traversal_attempt`
|
|
5. If yes: serves the file from the resolved absolute path
|
|
|
|
### Config Addition
|
|
|
|
| Env Var | Default | Purpose |
|
|
|---------|---------|---------|
|
|
| `REDFLAG_BINARY_STORAGE_PATH` | `./binaries` | Allowed directory for binary downloads |
|
|
|
|
Added to `Config` struct in `config.go`.
|
|
|
|
## Files Changed
|
|
|
|
| File | Change |
|
|
|------|--------|
|
|
| `migrations/030_add_operational_settings.up.sql` | NEW: Seeds 6 operational settings |
|
|
| `migrations/030_add_operational_settings.down.sql` | NEW: Removes operational settings |
|
|
| `cmd/server/main.go` | Reads settings from DB, passes to TimeoutService, uses in goroutines, `getOperationalSetting()` helper |
|
|
| `internal/services/timeout.go` | Constructor accepts durations + checkInterval field, `Start()` uses configurable interval |
|
|
| `internal/config/config.go` | Added `BinaryStoragePath` field |
|
|
| `internal/api/handlers/downloads.go` | Path sanitization before `c.File()` |
|
|
|
|
## Tests Added
|
|
|
|
| File | Tests |
|
|
|------|-------|
|
|
| `services/timeout_config_test.go` | `TestTimeoutServiceUsesConfiguredValues`, `TestTimeoutServiceFallsBackToDefaults`, `TestGetOperationalSettingFallsBackToDefault` |
|
|
| `handlers/downloads_security_test.go` | `TestDownloadsRejectsPathTraversal`, `TestDownloadsAcceptsSafePath`, `TestDownloadsRejectsSymlinkEscape` |
|
|
|
|
## Test Results
|
|
|
|
```
|
|
Server: 103 passed, 0 failed (7 packages)
|
|
Agent: 60 passed, 0 failed (10 packages)
|
|
Total: 163 tests, 0 failures
|
|
```
|