Code quality #25

Open
opened 2026-02-28 22:47:36 +00:00 by peio · 0 comments
Owner

High: The shared IdP cache is not concurrency-safe and can panic under real mail traffic. The cache is a plain map with no locking at internal/mailcloak/cache.go (line 5), while policy requests are handled in separate goroutines at internal/mailcloak/policy.go (line 48) and both provider clients read/write that cache at internal/mailcloak/keycloak.go (line 113), internal/mailcloak/keycloak.go (line 142), internal/mailcloak/authentik.go (line 114), and internal/mailcloak/authentik.go (line 129). Two concurrent lookups can produce a concurrent map read and map write crash.

High: A single unexpected Accept error can permanently disable one listener, and the main process will not notice or recover. Both servers exit on the first non-shutdown Accept error at internal/mailcloak/policy.go (line 38) and internal/mailcloak/socketmap.go (line 34). Start only records the error at internal/mailcloak/run.go (line 75) and internal/mailcloak/run.go (line 87), while the daemon entrypoint waits only for OS signals at cmd/mailcloak/main.go (line 41). That leaves the process alive but partially dead until an external restart.

Medium: Service shutdown leaks the opened SQLite handle. The DB is opened and stored at internal/mailcloak/run.go (line 57), but Service.Close only closes listeners at internal/mailcloak/run.go (line 111). In tests or embedded use that start/stop services repeatedly, this can keep WAL/FD state open longer than intended.

Medium: Socket startup blindly deletes the configured path before binding, even if it is not a stale socket. Both listeners call os.Remove(sock) unconditionally at internal/mailcloak/policy.go (line 20) and internal/mailcloak/socketmap.go (line 16). A bad config path can unlink an unrelated file before startup fails.

Medium: mailcloakctl exposes application secrets through process arguments and shell history. The CLI takes the app password as a positional argument at mailcloakctl (line 397), hashes it at mailcloakctl (line 304), and the README documents that pattern at README.md (line 149). On multi-user systems, that leaks credentials via ps, audit logs, and shell history.

High: The shared IdP cache is not concurrency-safe and can panic under real mail traffic. The cache is a plain map with no locking at internal/mailcloak/cache.go (line 5), while policy requests are handled in separate goroutines at internal/mailcloak/policy.go (line 48) and both provider clients read/write that cache at internal/mailcloak/keycloak.go (line 113), internal/mailcloak/keycloak.go (line 142), internal/mailcloak/authentik.go (line 114), and internal/mailcloak/authentik.go (line 129). Two concurrent lookups can produce a concurrent map read and map write crash. High: A single unexpected Accept error can permanently disable one listener, and the main process will not notice or recover. Both servers exit on the first non-shutdown Accept error at internal/mailcloak/policy.go (line 38) and internal/mailcloak/socketmap.go (line 34). Start only records the error at internal/mailcloak/run.go (line 75) and internal/mailcloak/run.go (line 87), while the daemon entrypoint waits only for OS signals at cmd/mailcloak/main.go (line 41). That leaves the process alive but partially dead until an external restart. Medium: Service shutdown leaks the opened SQLite handle. The DB is opened and stored at internal/mailcloak/run.go (line 57), but Service.Close only closes listeners at internal/mailcloak/run.go (line 111). In tests or embedded use that start/stop services repeatedly, this can keep WAL/FD state open longer than intended. Medium: Socket startup blindly deletes the configured path before binding, even if it is not a stale socket. Both listeners call os.Remove(sock) unconditionally at internal/mailcloak/policy.go (line 20) and internal/mailcloak/socketmap.go (line 16). A bad config path can unlink an unrelated file before startup fails. Medium: mailcloakctl exposes application secrets through process arguments and shell history. The CLI takes the app password as a positional argument at mailcloakctl (line 397), hashes it at mailcloakctl (line 304), and the README documents that pattern at README.md (line 149). On multi-user systems, that leaks credentials via ps, audit logs, and shell history.
peio added this to the mailcloak project 2026-02-28 22:47:37 +00:00
peio moved this to In Progress in mailcloak on 2026-02-28 22:47:39 +00:00
peio moved this to In Review in mailcloak on 2026-03-03 17:34:58 +00:00
Sign in to join this conversation.
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: peio/mailcloak#25
No description provided.