Skip to main content

max / multithreaded

7.7 KB · 130 lines History Blame Raw
1 # Multithreaded — Code Review
2
3 **Date:** 2026-04-12
4 **Version:** 0.3.2
5 **Reviewer:** Claude (Opus 4.6)
6 **Scope:** Full codebase review — all Rust source, SQL migrations, templates, CSS, JS, deploy config, tests
7
8 ## Summary
9
10 Multithreaded is a forum application (~8,800 LOC Rust, ~20,800 total) built on Axum/PostgreSQL with MNW OAuth integration. 3-crate workspace (mt-core, mt-db, main app). 21 migrations, 21 HTML templates, 510 lines JS, 1,761 lines CSS. 225+ tests (35 unit + 190 integration). 0 clippy warnings. Security posture is strong. Code is clean and well-organized.
11
12 **Overall: A** — consistent with the standing audit grade. No new vulnerabilities found. Several minor structural items noted below.
13
14 ---
15
16 ## Findings
17
18 ### [MEDIUM] Dependency advisories — 4 active
19
20 `cargo audit` reports 4 vulnerabilities:
21
22 | Crate | Advisory | Severity | Fix |
23 |-------|----------|----------|-----|
24 | aws-lc-sys 0.38.0 | RUSTSEC-2026-0044 | HIGH | `cargo update -p aws-lc-sys` (>=0.39.0) |
25 | aws-lc-sys 0.38.0 | RUSTSEC-2026-0048 | HIGH 7.4 | Same |
26 | rustls-webpki 0.103.9 | RUSTSEC-2026-0049 | — | `cargo update -p rustls-webpki` (>=0.103.10) |
27 | rand 0.8.5 + 0.9.2 | RUSTSEC-2026-0097 | — | Unsoundness with custom loggers (not applicable here, but upgrade when deps allow) |
28
29 Additionally 3 allowed warnings (rsa, lru — transitive, no direct fix available).
30
31 aws-lc-sys and rustls-webpki are straightforward `cargo update` fixes. The rand advisory (RUSTSEC-2026-0097) affects `rand::rng()` with custom loggers — MT doesn't use custom loggers, so no practical impact, but both 0.8.5 (direct dep) and 0.9.2 (transitive via axum/governor) are flagged. Consider bumping the direct `rand` dep from 0.8 to 0.9 when convenient.
32
33 ### [MEDIUM] Three route files exceed 500-line guideline
34
35 Per project conventions, files with >500 lines of branching logic should be split.
36
37 | File | Lines | Content |
38 |------|-------|---------|
39 | `src/routes/mod.rs` | 578 | Route tree, form types, 15+ helper functions |
40 | `src/routes/forum/views.rs` | 628 | 11 view handlers |
41 | `src/routes/forum/actions.rs` | 600 | 10 action handlers |
42
43 `mod.rs` could split helpers into `routes/helpers.rs` (validation, permission checks, markdown rendering) and keep forms + route tree in `mod.rs`. The forum files could split by page area (thread views vs. listing views, thread actions vs. post actions).
44
45 `queries.rs` (1,503) and `mutations.rs` (879) are exempt — flat lists of SQL query functions with no branching logic.
46
47 ### [LOW] Pagination partial not used consistently
48
49 `templates/pages/forum_directory.html` (lines 40-49) inlines pagination markup instead of using `{% include "partials/pagination.html" %}`. Only `thread.html` uses the partial. If the pagination design changes, it needs updating in two places.
50
51 ### [LOW] CSS: `.form-inline-row` has contradictory display properties
52
53 ```css
54 .form-inline-row { display: inline; gap: 0.25rem; flex-direction: row; align-items: center; }
55 ```
56
57 `gap`, `flex-direction`, and `align-items` are flex/grid properties — they have no effect with `display: inline`. Used on 2 admin forms. Should be `display: inline-flex`.
58
59 ### [LOW] No `Cache-Control` on HTML responses
60
61 Static files get caching from tower-http `ServeDir`. Image proxy sets `max-age=31536000`. But regular HTML pages have no explicit cache control headers. Adding `private, no-cache` for authenticated pages prevents stale content after logout/session change.
62
63 ### [INFO] Test harness duplication
64
65 `tests/harness/mod.rs`: `new()` and `new_with_admin()` share ~40 lines of identical setup code. `tests/harness/client.rs`: `send_raw` and `send_raw_with_token` duplicate ~60 lines. Not a correctness issue, but a maintenance burden.
66
67 ### [INFO] `MaybeUser` silently swallows session errors
68
69 In `auth.rs`, the `MaybeUser` extractor returns `MaybeUser(None)` on any session read error. This is intentionally infallible, but session store failures silently deauthenticate users without logging at the extraction point. A `tracing::warn!` on the error path would help debugging.
70
71 ### [INFO] No index on `posts.removed_at`
72
73 The moderation page queries removed posts and thread views filter by removal status. At scale, a partial index `WHERE removed_at IS NOT NULL` on `posts` would help. Current data volumes don't require it.
74
75 ---
76
77 ## Strengths
78
79 - **Security is defense-in-depth.** CSRF (constant-time comparison, auto-injected), SSRF (comprehensive private IP blocking on link previews), XSS (docengine sanitization + Askama autoescaping), SQL injection (all parameterized), rate limiting (per-IP + per-user), HMAC internal API with replay protection, EXIF stripping on uploads, CSP headers. 11 dedicated XSS tests.
80 - **Immutable post model.** Posts cannot be edited or deleted by users — only corrected via footnotes or mod-removed (content preserved). Philosophically consistent and well-executed.
81 - **Test infrastructure is excellent.** Per-test database isolation. Cookie-aware HTTP client with auto CSRF extraction. Full Axum app in-process. 22 workflow test files covering every feature area.
82 - **Clean 3-crate separation.** mt-core has zero deps beyond chrono. mt-db has no web framework deps. Main crate handles all HTTP concerns. No circular dependencies.
83 - **Production hardening.** Systemd service with 18 security directives (NoNewPrivileges, ProtectSystem=strict, ProtectHome, PrivateTmp, RestrictAddressFamilies, etc.). Memory cap 512M. Graceful shutdown.
84 - **Quote verification.** SHA-256 hash of quoted text verified server-side before rendering attribution. Prevents fabricated quotes.
85 - **Observability.** 86+ `#[instrument(skip_all)]` annotations. Structured logging via tracing with env filter.
86
87 ## Security Checklist
88
89 | Check | Status |
90 |-------|--------|
91 | SQL injection | Pass — all queries parameterized via SQLx |
92 | XSS | Pass — docengine sanitization + Askama autoescaping + CSP |
93 | CSRF | Pass — synchronizer token, constant-time comparison, all mutating routes |
94 | SSRF | Pass — link preview validates IPs (IPv4+IPv6 private ranges blocked) |
95 | Auth bypass | Pass — fail-closed access checks, session cycling on login |
96 | IDOR | Pass — all resource access checks use authenticated user context |
97 | Rate limiting | Pass — per-IP (tower-governor) + per-user (15 posts/60s) + upload rate (20/hr) |
98 | Secrets in source | Pass — env-based config, no secrets committed (env templates have placeholders) |
99 | Dependency advisories | Fail — 4 active (2 HIGH via aws-lc-sys, fixable with cargo update) |
100
101 ## Metrics
102
103 | Metric | Value |
104 |--------|-------|
105 | Rust source LOC | ~8,800 |
106 | Total LOC (all files) | ~20,800 |
107 | Rust source files | 39 |
108 | Test files | 24 (harness + workflows) |
109 | Test count | 225+ (35 unit + 190 integration) |
110 | Tests/KLOC | ~26 |
111 | Clippy warnings | 0 |
112 | Migrations | 21 |
113 | HTML templates | 21 |
114 | Query functions | 55+ |
115 | Mutation functions | 30+ |
116 | Dependencies (direct) | 28 |
117 | Audit advisories | 4 (2 HIGH, 2 low/info) |
118
119 ## Action Items
120
121 1. ~~**[MEDIUM]** Run `cargo update -p aws-lc-sys -p rustls-webpki` to fix 3 of 4 advisories~~ — Done. aws-lc-sys 0.39.1, rustls-webpki 0.103.11.
122 2. ~~**[MEDIUM]** Split `routes/mod.rs` helpers into `routes/helpers.rs`~~ — Done. mod.rs 281, helpers.rs 315.
123 3. ~~**[LOW]** Fix `.form-inline-row` CSS to `display: inline-flex`~~ — Done.
124 4. ~~**[LOW]** Use pagination partial in `forum_directory.html`~~ — Done.
125 5. ~~**[LOW]** Add `Cache-Control: private, no-cache` to HTML responses~~ — Done.
126 6. ~~**[MEDIUM]** Split `forum/views.rs` and `forum/actions.rs`~~ — Done. views.rs 424, thread.rs 224, posts.rs 443, actions.rs 172.
127 7. **[INFO]** Transitive dep advisories (rand, rsa, lru) — no fix available, monitor upstream.
128 8. **[INFO]** Add partial index on `posts.removed_at` when data volume warrants it.
129 9. **[INFO]** Add `tracing::warn!` to `MaybeUser` extractor on session read errors.
130