Add comprehensive config validation and race testing target

Split config validation into validateFields() (pure logic) and
validateFiles() (filesystem checks) for testability. New validations:
TLS file existence, token TTL parseability/positivity, Argon2 params > 0,
valid log level, non-empty listen addresses. Added 18 tests covering all
validation paths. Added `make test-race` target. Resolves A-015 and A-017.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-03-24 20:49:16 -07:00
parent c5469c6bdf
commit 41839b7284
4 changed files with 307 additions and 7 deletions

View File

@@ -137,10 +137,13 @@ severity levels. All critical and high issues resolved.
### A-015: Incomplete Config Validation ### A-015: Incomplete Config Validation
- **Severity**: Medium - **Severity**: Medium
- **Status**: Open - **Status**: ~~Resolved~~
- **Description**: TLS files not checked for existence at startup. - **Description**: TLS files not checked for existence at startup.
Token TTL, WebAuthn config not validated. Token TTL, WebAuthn config not validated.
- **Plan**: Add file existence checks and config field validation. - **Resolution**: Split validation into `validateFields()` and
`validateFiles()`. Added checks for: TLS file existence, token TTL
parseability and positivity, Argon2 params > 0, valid log level,
non-empty listen addresses. Full test coverage in config_test.go.
### A-016: Inconsistent Error Types ### A-016: Inconsistent Error Types
- **Severity**: Low - **Severity**: Low
@@ -150,9 +153,9 @@ severity levels. All critical and high issues resolved.
### A-017: No Race Condition Testing ### A-017: No Race Condition Testing
- **Severity**: Low - **Severity**: Low
- **Status**: Open - **Status**: ~~Resolved~~
- **Description**: Test suite does not use `-race` flag. - **Description**: Test suite does not use `-race` flag.
- **Plan**: Add `make test-race` target. - **Resolution**: Added `make test-race` target that runs `go test -race ./...`.
--- ---
@@ -174,9 +177,9 @@ severity levels. All critical and high issues resolved.
| A-012 | Medium | Accepted | | A-012 | Medium | Accepted |
| A-013 | Medium | Accepted | | A-013 | Medium | Accepted |
| A-014 | Medium | Open | | A-014 | Medium | Open |
| A-015 | Medium | Open | | A-015 | Medium | ~~Resolved~~ |
| A-016 | Low | Open | | A-016 | Low | Open |
| A-017 | Low | Open | | A-017 | Low | ~~Resolved~~ |
## Design Note: Single-User Model ## Design Note: Single-User Model

View File

@@ -1,4 +1,4 @@
.PHONY: build test vet lint proto proto-lint clean all .PHONY: build test test-race vet lint proto proto-lint clean all
LDFLAGS := -trimpath -ldflags="-s -w -X main.version=$(shell git describe --tags --always --dirty 2>/dev/null || echo dev)" LDFLAGS := -trimpath -ldflags="-s -w -X main.version=$(shell git describe --tags --always --dirty 2>/dev/null || echo dev)"
@@ -11,6 +11,9 @@ build:
test: test:
go test ./... go test ./...
test-race:
go test -race ./...
vet: vet:
go vet ./... go vet ./...

View File

@@ -70,11 +70,65 @@ func Load(path string) (*Config, error) {
} }
func (c *Config) validate() error { func (c *Config) validate() error {
if err := c.validateFields(); err != nil {
return err
}
if err := c.validateFiles(); err != nil {
return err
}
return nil
}
// validateFields checks config values that don't require filesystem access.
func (c *Config) validateFields() error {
if c.Database.Path == "" { if c.Database.Path == "" {
return fmt.Errorf("database.path is required") return fmt.Errorf("database.path is required")
} }
if c.Server.TLSCert == "" || c.Server.TLSKey == "" { if c.Server.TLSCert == "" || c.Server.TLSKey == "" {
return fmt.Errorf("server.tls_cert and server.tls_key are required") return fmt.Errorf("server.tls_cert and server.tls_key are required")
} }
if c.Server.ListenAddr == "" {
return fmt.Errorf("server.listen_addr is required")
}
if c.Server.GRPCAddr == "" {
return fmt.Errorf("server.grpc_addr is required")
}
d, err := c.Auth.TokenDuration()
if err != nil {
return fmt.Errorf("auth.token_ttl is invalid: %w", err)
}
if d <= 0 {
return fmt.Errorf("auth.token_ttl must be positive")
}
if c.Auth.Argon2Memory == 0 {
return fmt.Errorf("auth.argon2_memory must be greater than zero")
}
if c.Auth.Argon2Time == 0 {
return fmt.Errorf("auth.argon2_time must be greater than zero")
}
if c.Auth.Argon2Threads == 0 {
return fmt.Errorf("auth.argon2_threads must be greater than zero")
}
switch c.Log.Level {
case "debug", "info", "warn", "error":
// valid
default:
return fmt.Errorf("log.level must be one of: debug, info, warn, error (got %q)", c.Log.Level)
}
return nil
}
// validateFiles checks that referenced files exist on disk.
func (c *Config) validateFiles() error {
if _, err := os.Stat(c.Server.TLSCert); err != nil {
return fmt.Errorf("server.tls_cert file not found: %w", err)
}
if _, err := os.Stat(c.Server.TLSKey); err != nil {
return fmt.Errorf("server.tls_key file not found: %w", err)
}
return nil return nil
} }

View File

@@ -0,0 +1,240 @@
package config
import (
"os"
"path/filepath"
"testing"
)
// validConfig returns a Config with all fields set to valid values.
// TLS cert/key paths must be overridden by the caller for file validation tests.
func validConfig() Config {
return Config{
Server: ServerConfig{
ListenAddr: ":8443",
GRPCAddr: ":9443",
TLSCert: "/tmp/cert.pem",
TLSKey: "/tmp/key.pem",
},
Database: DatabaseConfig{
Path: "/srv/eng-pad/eng-pad.db",
},
Auth: AuthConfig{
TokenTTL: "24h",
Argon2Memory: 65536,
Argon2Time: 3,
Argon2Threads: 4,
},
Log: LogConfig{
Level: "info",
},
}
}
func TestValidateFields_ValidConfig(t *testing.T) {
cfg := validConfig()
if err := cfg.validateFields(); err != nil {
t.Fatalf("valid config should pass field validation: %v", err)
}
}
func TestValidateFields_MissingDatabasePath(t *testing.T) {
cfg := validConfig()
cfg.Database.Path = ""
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for missing database.path")
}
}
func TestValidateFields_MissingTLSCert(t *testing.T) {
cfg := validConfig()
cfg.Server.TLSCert = ""
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for missing tls_cert")
}
}
func TestValidateFields_MissingTLSKey(t *testing.T) {
cfg := validConfig()
cfg.Server.TLSKey = ""
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for missing tls_key")
}
}
func TestValidateFields_MissingListenAddr(t *testing.T) {
cfg := validConfig()
cfg.Server.ListenAddr = ""
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for missing listen_addr")
}
}
func TestValidateFields_MissingGRPCAddr(t *testing.T) {
cfg := validConfig()
cfg.Server.GRPCAddr = ""
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for missing grpc_addr")
}
}
func TestValidateFields_InvalidTokenTTL(t *testing.T) {
cfg := validConfig()
cfg.Auth.TokenTTL = "not-a-duration"
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for invalid token_ttl")
}
}
func TestValidateFields_NegativeTokenTTL(t *testing.T) {
cfg := validConfig()
cfg.Auth.TokenTTL = "-1h"
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for negative token_ttl")
}
}
func TestValidateFields_ZeroTokenTTL(t *testing.T) {
cfg := validConfig()
cfg.Auth.TokenTTL = "0s"
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for zero token_ttl")
}
}
func TestValidateFields_ZeroArgon2Memory(t *testing.T) {
cfg := validConfig()
cfg.Auth.Argon2Memory = 0
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for zero argon2_memory")
}
}
func TestValidateFields_ZeroArgon2Time(t *testing.T) {
cfg := validConfig()
cfg.Auth.Argon2Time = 0
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for zero argon2_time")
}
}
func TestValidateFields_ZeroArgon2Threads(t *testing.T) {
cfg := validConfig()
cfg.Auth.Argon2Threads = 0
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for zero argon2_threads")
}
}
func TestValidateFields_InvalidLogLevel(t *testing.T) {
cfg := validConfig()
cfg.Log.Level = "trace"
if err := cfg.validateFields(); err == nil {
t.Fatal("expected error for invalid log level")
}
}
func TestValidateFields_AllLogLevels(t *testing.T) {
for _, level := range []string{"debug", "info", "warn", "error"} {
t.Run(level, func(t *testing.T) {
cfg := validConfig()
cfg.Log.Level = level
if err := cfg.validateFields(); err != nil {
t.Fatalf("log level %q should be valid: %v", level, err)
}
})
}
}
func TestValidateFiles_CertAndKeyExist(t *testing.T) {
dir := t.TempDir()
certPath := filepath.Join(dir, "cert.pem")
keyPath := filepath.Join(dir, "key.pem")
if err := os.WriteFile(certPath, []byte("cert"), 0600); err != nil {
t.Fatalf("write cert: %v", err)
}
if err := os.WriteFile(keyPath, []byte("key"), 0600); err != nil {
t.Fatalf("write key: %v", err)
}
cfg := validConfig()
cfg.Server.TLSCert = certPath
cfg.Server.TLSKey = keyPath
if err := cfg.validateFiles(); err != nil {
t.Fatalf("expected no error when cert/key files exist: %v", err)
}
}
func TestValidateFiles_MissingCertFile(t *testing.T) {
dir := t.TempDir()
keyPath := filepath.Join(dir, "key.pem")
if err := os.WriteFile(keyPath, []byte("key"), 0600); err != nil {
t.Fatalf("write key: %v", err)
}
cfg := validConfig()
cfg.Server.TLSCert = filepath.Join(dir, "nonexistent-cert.pem")
cfg.Server.TLSKey = keyPath
if err := cfg.validateFiles(); err == nil {
t.Fatal("expected error for missing cert file")
}
}
func TestValidateFiles_MissingKeyFile(t *testing.T) {
dir := t.TempDir()
certPath := filepath.Join(dir, "cert.pem")
if err := os.WriteFile(certPath, []byte("cert"), 0600); err != nil {
t.Fatalf("write cert: %v", err)
}
cfg := validConfig()
cfg.Server.TLSCert = certPath
cfg.Server.TLSKey = filepath.Join(dir, "nonexistent-key.pem")
if err := cfg.validateFiles(); err == nil {
t.Fatal("expected error for missing key file")
}
}
func TestLoad_ValidTOML(t *testing.T) {
dir := t.TempDir()
certPath := filepath.Join(dir, "cert.pem")
keyPath := filepath.Join(dir, "key.pem")
if err := os.WriteFile(certPath, []byte("cert"), 0600); err != nil {
t.Fatalf("write cert: %v", err)
}
if err := os.WriteFile(keyPath, []byte("key"), 0600); err != nil {
t.Fatalf("write key: %v", err)
}
toml := `
[server]
listen_addr = ":8443"
grpc_addr = ":9443"
tls_cert = "` + certPath + `"
tls_key = "` + keyPath + `"
[database]
path = "/srv/eng-pad/eng-pad.db"
[auth]
token_ttl = "24h"
argon2_memory = 65536
argon2_time = 3
argon2_threads = 4
[log]
level = "info"
`
configPath := filepath.Join(dir, "config.toml")
if err := os.WriteFile(configPath, []byte(toml), 0600); err != nil {
t.Fatalf("write config: %v", err)
}
cfg, err := Load(configPath)
if err != nil {
t.Fatalf("load valid config: %v", err)
}
if cfg.Server.ListenAddr != ":8443" {
t.Fatalf("got listen_addr %q, want %q", cfg.Server.ListenAddr, ":8443")
}
}