[audit] Security, AX compliance, missing tests, error handling #2

New issue

Open

opened 2026-03-22 16:40:59 +00:00 by Virgil · 3 comments

Virgil commented

2026-03-22 16:40:59 +00:00

Member

Full audit:

Security: path traversal, injection, panics on untrusted input, race conditions
AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E
Missing tests: exported functions without test coverage
Error handling: silently dropped errors, bare panics, missing nil checks
UK English: American spellings in comments/docs
Missing usage-example comments on exported identifiers
Missing SPDX licence headers

Report all findings with severity and file:line. Do NOT fix.

Full audit: 1. Security: path traversal, injection, panics on untrusted input, race conditions 2. AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E 3. Missing tests: exported functions without test coverage 4. Error handling: silently dropped errors, bare panics, missing nil checks 5. UK English: American spellings in comments/docs 6. Missing usage-example comments on exported identifiers 7. Missing SPDX licence headers Report all findings with severity and file:line. Do NOT fix.

Virgil commented

2026-03-22 18:16:29 +00:00

Author

Member

Codex Audit Findings

CRITICAL (1)

TLS verification never set — constructor-time requests run with certs disabled even when insecure=false (unifi/client.go:21, :38)

HIGH (3)

New() replaces SDK http.Client after login, drops cookie jar — auth fails on subsequent requests (client.go:29, :43)
GetNetworks interpolates raw siteName into path — path injection (networks.go:49)
SaveConfig calls Set() but never Commit() — silently never persists to disk (config.go:108, :144)

MEDIUM (1)

ResolveConfig suppresses config-load failures, falls back to defaults with err=nil (config.go:56)

## Codex Audit Findings ### CRITICAL (1) 1. TLS verification never set — constructor-time requests run with certs disabled even when insecure=false (unifi/client.go:21, :38) ### HIGH (3) 2. New() replaces SDK http.Client after login, drops cookie jar — auth fails on subsequent requests (client.go:29, :43) 3. GetNetworks interpolates raw siteName into path — path injection (networks.go:49) 4. SaveConfig calls Set() but never Commit() — silently never persists to disk (config.go:108, :144) ### MEDIUM (1) 5. ResolveConfig suppresses config-load failures, falls back to defaults with err=nil (config.go:56)

Virgil commented

2026-03-23 06:43:44 +00:00

Author

Member

Fix Applied

Commit 7b35b06: fix(unifi): address issue 2 audit findings

Fixed TLS verification (set before constructor login)
Preserved SDK http.Client cookie jar after login
Escaped siteName in GetNetworks path
SaveConfig now calls Commit()
Added 117+ lines of new tests (client_test.go, config_test.go, networks_test.go)

## Fix Applied Commit 7b35b06: fix(unifi): address issue 2 audit findings - Fixed TLS verification (set before constructor login) - Preserved SDK http.Client cookie jar after login - Escaped siteName in GetNetworks path - SaveConfig now calls Commit() - Added 117+ lines of new tests (client_test.go, config_test.go, networks_test.go)

Virgil commented

2026-03-23 09:08:00 +00:00

Author

Member

Verification: FAIL (deeper issue found)

HIGH: SaveConfig fix (Commit) now WORKS — but exposes that config stack writes secrets to world-readable ~/.core/config.yaml. Traced through: config.go:227 → go-io local/client.go:135 → os.WriteFile with default perms.

This is an upstream issue in go-io/config, not go-netops. The fix is correct but the underlying write path needs 0600 permissions for config files containing secrets.

## Verification: FAIL (deeper issue found) HIGH: SaveConfig fix (Commit) now WORKS — but exposes that config stack writes secrets to world-readable ~/.core/config.yaml. Traced through: config.go:227 → go-io local/client.go:135 → os.WriteFile with default perms. This is an upstream issue in go-io/config, not go-netops. The fix is correct but the underlying write path needs 0600 permissions for config files containing secrets.