fix(security): correctness & security hardening from full code review#1
Merged
Conversation
IMAP (highest-risk hand-rolled code): - fix read-only data-loss: EXAMINE + STORE \Deleted + CLOSE deleted mail from a read-only-opened mailbox; track writability per selection and gate STORE \Deleted / EXPUNGE / CLOSE-expunge on it - add pre-auth (60s) and authenticated/IDLE (30min) read timeouts so dead connections no longer exhaust the session slots (slowloris) - fail fast on a configured-but-missing TLS cert instead of silently falling back to plaintext credentials - brute-force cap, credential redaction in logs, literal-size caps, and a match timeout on the LIST wildcard->regex conversion Data layer: - JSON store: replace NFS-unreliable mtime dedup with a state-aware tie-break; add a GUID fallback for folder-name collisions - resolver: fix torn read of the catch-all cache (volatile record ref) and add Invalidate() wired to alias create/delete so a new alias accepts mail immediately instead of after the 30s negative-cache TTL - SQLite: bound the exclusive-connection gate wait (60s) so a re-entrant or stuck open throws instead of hanging the app forever Web: - tighten the auth cookie to SameSite=Strict (closes the residual CSRF window on the route-only admin POSTs) Tests: add regression coverage for the IMAP read-only guard and the resolver cache invalidation. Full suite green (100 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full correctness + security code review of the project. All fixes on
fix/code-review; Release build clean, 100 tests pass (2 new regression tests).Fixed — high severity
EXAMINE+STORE \Deleted+CLOSEdeleted mail from a read-only-opened mailbox. Writability is now tracked per-selection and gatesSTORE \Deleted/EXPUNGE/CLOSE-expunge.Fixed — medium severity
volatilerecord ref for the catch-all cache;IAliasResolver.Invalidate()wired to alias create/delete so new aliases accept mail immediately (was up to 30s late).TimeoutExceptioninstead of a silent hang.LISTregex match-timeout (ReDoS guard).Behavior change (review before merge)
SameSite=Lax->Strict— closes the residual CSRF window on route-only admin POSTs. Trade-off: following an external link into the UI shows the login page until you navigate within the app. Revert the one line if the UX is undesirable.Reviewed, deliberately unchanged (false positives)
SMTP governor
>(correct),ImapFormat.BodyStartbound (correct), reindexer catch-all routing (correct for recovery), message-HTML CSP (already airtight viadefault-src 'none'+sandbox=""), streaming FETCH literals (perf note, not a bug).🤖 Generated with Claude Code