Skip to main content

max / makenotwork

Remediate audit findings: SSRF userinfo bypass, guest role, C1 scope gaps Same-day remediation of the 2026-07-04 audit (H1/M1/M2, L1-L6): - H1: reject userinfo-prefixed literal IPs in the link_preview SSRF guard (strip userinfo before the host split) and widen is_private_ip reserved ranges (0/8, 192.0.0.0/24, 198.18/15, 240/4, NAT64, v4-compatible IPv6) - M1: drop 'guest' from memberships_role_check so the schema can't hold a role CommunityRole cannot decode (migration 033) - M2: scope category and thread-tracking loads to the URL slug's community via CommunityScope, closing the residual C1 cross-community gaps - L1-L6: auth.rs PKCE/nonce unit tests; concurrency tests; delete dead templates/partials.rs; move logout csrf_input inside the form; log outbound-fetch failures; host-parse the loopback exemption in config Post-fix: cargo check/clippy clean, tests green.
Co-Authored-By
Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-04 23:04 UTC
Signed with PGP, not checked
Commit: 8e7b5fded71e9d7c10d0f4a02074994f0cdd6b08
Parent: bfd4533
30 files changed, +638 insertions, -522 deletions
M multithreaded/Cargo.lock +1
@@ -3062,6 +3062,7 @@
3062 3062 dependencies = [
3063 3063 "aws-config",
3064 3064 "aws-sdk-s3",
3065 + "bytes",
3065 3066 "tokio",
3066 3067 "tracing",
3067 3068 ]
M multithreaded/src/auth.rs +84
@@ -166,6 +166,31 @@
166 166 }
167 167 }
168 168
169 + /// Axum extractor that requires an authenticated session.
170 + ///
171 + /// Yields the [`SessionUser`] directly, or rejects with a redirect to
172 + /// `/auth/login` — the exact behaviour ~30 write/settings handlers previously
173 + /// open-coded as `session_user.ok_or_else(|| Redirect::to("/auth/login")…)?`.
174 + /// Use this instead of `MaybeUser` whenever the handler needs a logged-in user.
175 + pub struct RequireUser(pub SessionUser);
176 +
177 + impl FromRequestParts<AppState> for RequireUser {
178 + type Rejection = axum::response::Response;
179 +
180 + async fn from_request_parts(
181 + parts: &mut Parts,
182 + state: &AppState,
183 + ) -> Result<Self, Self::Rejection> {
184 + let session = Session::from_request_parts(parts, state)
185 + .await
186 + .expect("session layer missing");
187 + let user = SessionUser::from_session(&session)
188 + .await
189 + .ok_or_else(|| Redirect::to("/auth/login").into_response())?;
190 + Ok(RequireUser(user))
191 + }
192 + }
193 +
169 194 /// Axum extractor that requires the user to be the platform admin.
170 195 /// Returns 404 to non-admins (hides admin routes).
171 196 pub struct PlatformAdmin(pub SessionUser);
@@ -771,3 +796,62 @@
771 796 }
772 797 Redirect::to("/")
773 798 }
799 +
800 + #[cfg(test)]
801 + mod tests {
802 + use super::*;
803 +
804 + #[test]
805 + fn pkce_challenge_matches_rfc7636_test_vector() {
806 + // RFC 7636 Appendix B known-answer vector.
807 + let verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk";
808 + let challenge = pkce_challenge(verifier);
809 + assert_eq!(challenge, "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM");
810 + }
811 +
812 + #[test]
813 + fn pkce_challenge_is_deterministic() {
814 + let v = generate_verifier();
815 + assert_eq!(pkce_challenge(&v), pkce_challenge(&v));
816 + }
817 +
818 + #[test]
819 + fn verifier_is_url_safe_base64_of_32_bytes() {
820 + let v = generate_verifier();
821 + // 32 bytes → 43 chars of unpadded base64url.
822 + assert_eq!(v.len(), 43);
823 + assert!(!v.contains('='), "must be unpadded");
824 + assert!(
825 + v.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_'),
826 + "must be url-safe: {v}"
827 + );
828 + // And the challenge is likewise url-safe/unpadded (sent as a query param).
829 + let c = pkce_challenge(&v);
830 + assert_eq!(c.len(), 43);
831 + assert!(!c.contains('='));
832 + }
833 +
834 + #[test]
835 + fn verifier_and_nonce_are_unpredictable() {
836 + // Sanity that we're not returning a constant. Collisions across 32/16
837 + // random bytes are astronomically unlikely, so equality means a bug.
838 + assert_ne!(generate_verifier(), generate_verifier());
839 + assert_ne!(generate_state_nonce(), generate_state_nonce());
840 + }
841 +
842 + #[test]
843 + fn state_nonce_is_128_bits_of_hex() {
844 + let n = generate_state_nonce();
845 + assert_eq!(n.len(), 32); // 16 bytes → 32 hex chars
846 + assert!(n.bytes().all(|b| b.is_ascii_hexdigit()));
847 + }
848 +
849 + #[test]
850 + fn state_comparison_is_constant_time_and_correct() {
851 + // The callback compares the returned `state` against the session nonce via
852 + // this shared constant-time primitive (auth.rs). Guard the wiring here.
853 + assert!(crate::csrf::constant_time_compare("abc123", "abc123"));
854 + assert!(!crate::csrf::constant_time_compare("abc123", "abc124"));
855 + assert!(!crate::csrf::constant_time_compare("abc", "abc123"));
856 + }
857 + }
M multithreaded/src/config.rs +60 -3
@@ -80,14 +80,45 @@
80 80 /// `localhost`, `[::1]`) are exempt so local HTTP development still works;
81 81 /// anything else must be `https`.
82 82 fn assert_secure_url(var_name: &str, url: &str) {
83 - let is_loopback =
84 - url.contains("127.0.0.1") || url.contains("localhost") || url.contains("[::1]");
85 83 assert!(
86 - is_loopback || url.starts_with("https://"),
84 + is_loopback_url(url) || url.starts_with("https://"),
87 85 "{var_name} must be https for a non-loopback deployment (got `{url}`)"
88 86 );
89 87 }
90 88
89 + /// Whether an http(s) URL's host is a loopback literal.
90 + ///
91 + /// Parses the host exactly rather than substring-matching: a naive
92 + /// `url.contains("127.0.0.1")` would treat `http://127.0.0.1.attacker.com` as
93 + /// loopback and boot it over cleartext. We strip scheme, userinfo, and port, then
94 + /// match the bare host against the loopback set (any `127.0.0.0/8`, `localhost`,
95 + /// `::1`).
96 + fn is_loopback_url(url: &str) -> bool {
97 + let after_scheme =
98 + match url.strip_prefix("http://").or_else(|| url.strip_prefix("https://")) {
99 + Some(rest) => rest,
100 + None => return false,
101 + };
102 + let authority = after_scheme.split('/').next().unwrap_or("");
103 + let host_and_port = match authority.rsplit_once('@') {
104 + Some((_userinfo, host)) => host,
105 + None => authority,
106 + };
107 + let host = if let Some(rest) = host_and_port.strip_prefix('[') {
108 + // `[::1]:port` → `::1`
109 + rest.split(']').next().unwrap_or("")
110 + } else {
111 + // `host:port` → `host`
112 + host_and_port.split(':').next().unwrap_or("")
113 + };
114 + if host == "localhost" || host == "::1" {
115 + return true;
116 + }
117 + host.parse::<IpAddr>()
118 + .map(|ip| ip.is_loopback())
119 + .unwrap_or(false)
120 + }
121 +
91 122 /// Parse `TRUSTED_PROXIES` (comma-separated IPs). Unset → loopback only; an
92 123 /// explicit empty value → trust no proxy (every request keys on its peer).
93 124 /// Unparseable entries are skipped with a warning rather than failing boot.
@@ -126,6 +157,32 @@
126 157 assert_eq!(parse_trusted_proxies(Some(" ")).len(), 0);
127 158 }
128 159
160 + #[test]
161 + fn loopback_url_detection_is_exact() {
162 + assert!(is_loopback_url("http://127.0.0.1:3000/auth/callback"));
163 + assert!(is_loopback_url("http://127.0.0.5"));
164 + assert!(is_loopback_url("http://localhost:8080"));
165 + assert!(is_loopback_url("http://[::1]:3400/auth/callback"));
166 + assert!(is_loopback_url("http://user@127.0.0.1/"));
167 + // The substring-spoof the old check let through must now be rejected.
168 + assert!(!is_loopback_url("http://127.0.0.1.attacker.com/"));
169 + assert!(!is_loopback_url("http://localhost.evil.com/"));
170 + assert!(!is_loopback_url("http://example.com/"));
171 + assert!(!is_loopback_url("https://makenot.work/"));
172 + }
173 +
174 + #[test]
175 + #[should_panic]
176 + fn non_https_public_url_panics() {
177 + assert_secure_url("MNW_BASE_URL", "http://127.0.0.1.attacker.com/");
178 + }
179 +
180 + #[test]
181 + fn https_and_loopback_urls_boot() {
182 + assert_secure_url("MNW_BASE_URL", "https://makenot.work");
183 + assert_secure_url("OAUTH_REDIRECT_URI", "http://127.0.0.1:3400/auth/callback");
184 + }
185 +
129 186 #[test]
130 187 fn parses_list_and_skips_garbage() {
131 188 let p = parse_trusted_proxies(Some("100.64.0.1, garbage, 10.0.0.2"));
M multithreaded/src/link_preview.rs +95 -9
@@ -13,19 +13,32 @@
13 13 fn is_private_ip(ip: std::net::IpAddr) -> bool {
14 14 match ip {
15 15 std::net::IpAddr::V4(v4) => {
16 + let o = v4.octets();
16 17 v4.is_loopback()
17 18 || v4.is_private()
18 19 || v4.is_link_local()
19 20 || v4.is_broadcast()
20 21 || v4.is_unspecified()
21 - || v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64 // 100.64.0.0/10 (CGNAT / Tailscale)
22 + || o[0] == 0 // 0.0.0.0/8 "this network"
23 + || o[0] == 100 && (o[1] & 0xC0) == 64 // 100.64.0.0/10 (CGNAT / Tailscale)
24 + || o[0] == 192 && o[1] == 0 && o[2] == 0 // 192.0.0.0/24 IETF protocol assignments
25 + || o[0] == 198 && (o[1] & 0xFE) == 18 // 198.18.0.0/15 benchmarking
26 + || o[0] >= 240 // 240.0.0.0/4 reserved / experimental
22 27 }
23 28 std::net::IpAddr::V6(v6) => {
29 + let seg = v6.segments();
24 30 v6.is_loopback()
25 31 || v6.is_unspecified()
26 - || (v6.segments()[0] & 0xfe00) == 0xfc00 // ULA fd00::/7
27 - || (v6.segments()[0] & 0xffc0) == 0xfe80 // link-local
28 - || matches!(v6.to_ipv4_mapped(), Some(v4) if is_private_ip(std::net::IpAddr::V4(v4)))
32 + || (seg[0] & 0xfe00) == 0xfc00 // ULA fd00::/7
33 + || (seg[0] & 0xffc0) == 0xfe80 // link-local
34 + // NAT64 64:ff9b::/96 embeds a v4 address — check the embedded v4.
35 + || (seg[0] == 0x0064 && seg[1] == 0xff9b
36 + && is_private_ip(std::net::IpAddr::V4(std::net::Ipv4Addr::new(
37 + (seg[6] >> 8) as u8, seg[6] as u8, (seg[7] >> 8) as u8, seg[7] as u8,
38 + ))))
39 + // v4-mapped (::ffff:a.b.c.d) and v4-compatible (::a.b.c.d) both
40 + // carry an embedded v4 that reqwest may connect to directly.
41 + || matches!(v6.to_ipv4(), Some(v4) if is_private_ip(std::net::IpAddr::V4(v4)))
29 42 }
30 43 }
31 44 }
@@ -44,7 +57,21 @@
44 57 .strip_prefix("http://")
45 58 .or_else(|| lower.strip_prefix("https://"))
46 59 .unwrap_or("");
47 - let host_and_port = host_part.split('/').next().unwrap_or("");
60 + let authority = host_part.split('/').next().unwrap_or("");
61 + // Strip any userinfo ("user:pass@" or bare "@") — the real host is whatever
62 + // follows the LAST '@'. This must happen before the host/port split: without
63 + // it, `http://@10.0.0.1/` leaves `@10.0.0.1` as the "host", which fails
64 + // `IpAddr::parse` and slips past the literal-IP guard below. reqwest connects
65 + // to IP literals directly (it does NOT consult the custom `SsrfSafeResolver`
66 + // for a literal-IP host), so the literal-IP check here is the ONLY guard on
67 + // that path — the userinfo form must not be allowed to hide the IP from it.
68 + let host_and_port = match authority.rsplit_once('@') {
69 + Some((_userinfo, host)) => host,
70 + None => authority,
71 + };
72 + if host_and_port.is_empty() {
73 + return false;
74 + }
48 75 // Split host and optional port (`[ipv6]:port` or `host:port`).
49 76 let (host, port) = if host_and_port.starts_with('[') {
50 77 match host_and_port.split_once(']') {
@@ -186,18 +213,26 @@
186 213 url: &str,
187 214 ) -> Option<(Option<String>, Option<String>)> {
188 215 if !validate_url(url) {
216 + tracing::warn!(%url, "link preview blocked: url failed scheme/host SSRF validation");
189 217 return None;
190 218 }
191 219
192 - let resp = http
220 + let resp = match http
193 221 .get(url)
194 222 .timeout(std::time::Duration::from_secs(5))
195 223 .header("User-Agent", "Multithreaded/LinkPreview")
196 224 .send()
197 225 .await
198 - .ok()?;
226 + {
227 + Ok(r) => r,
228 + Err(e) => {
229 + tracing::debug!(%url, error = ?e, "link preview fetch failed (transport/timeout/blocked resolver)");
230 + return None;
231 + }
232 + };
199 233
200 234 if !resp.status().is_success() {
235 + tracing::debug!(%url, status = %resp.status(), "link preview fetch: non-success status");
201 236 return None;
202 237 }
203 238
@@ -263,18 +298,26 @@
263 298 #[tracing::instrument(skip_all)]
264 299 pub async fn fetch_image(http: &reqwest::Client, url: &str) -> Option<(Vec<u8>, &'static str)> {
265 300 if !validate_url(url) {
301 + tracing::warn!(%url, "image proxy blocked: url failed scheme/host SSRF validation");
266 302 return None;
267 303 }
268 304
269 - let resp = http
305 + let resp = match http
270 306 .get(url)
271 307 .timeout(std::time::Duration::from_secs(5))
272 308 .header("User-Agent", "Multithreaded/ImageProxy")
273 309 .send()
274 310 .await
275 - .ok()?;
311 + {
312 + Ok(r) => r,
313 + Err(e) => {
314 + tracing::debug!(%url, error = ?e, "image proxy fetch failed (transport/timeout/blocked resolver)");
315 + return None;
316 + }
317 + };
276 318
277 319 if !resp.status().is_success() {
320 + tracing::debug!(%url, status = %resp.status(), "image proxy fetch: non-success status");
278 321 return None;
279 322 }
280 323
@@ -500,6 +543,49 @@
500 543 assert!(validate_url("https://93.184.216.34"));
501 544 }
502 545
546 + #[test]
547 + fn validate_url_blocks_userinfo_literal_ip() {
548 + // H1: a userinfo-prefixed literal IP must not slip past the literal-IP
549 + // guard. reqwest connects straight to the IP (skipping SsrfSafeResolver),
550 + // so validate_url is the only defense on this path.
551 + assert!(!validate_url("http://@10.0.0.1/"));
552 + assert!(!validate_url("http://@169.254.169.254/")); // cloud metadata
553 + assert!(!validate_url("http://user:pass@127.0.0.1/"));
554 + assert!(!validate_url("http://foo@192.168.1.1:80/path"));
555 + assert!(!validate_url("http://a@b@10.0.0.1/")); // last '@' wins
556 + assert!(!validate_url("http://@[::1]/"));
557 + // A userinfo-prefixed *public* host is still allowed (host resolves and is
558 + // re-checked at connect time by SsrfSafeResolver).
559 + assert!(validate_url("http://user@example.com/"));
560 + }
561 +
562 + #[test]
563 + fn validate_url_blocks_empty_host() {
564 + assert!(!validate_url("http://@/"));
565 + assert!(!validate_url("http:///path"));
566 + }
567 +
568 + #[test]
569 + fn validate_url_blocks_reserved_ranges() {
570 + assert!(!validate_url("http://0.1.2.3")); // 0.0.0.0/8
571 + assert!(!validate_url("http://192.0.0.1")); // 192.0.0.0/24
572 + assert!(!validate_url("http://198.18.0.1")); // benchmarking 198.18/15
573 + assert!(!validate_url("http://198.19.255.255"));
574 + assert!(!validate_url("http://240.0.0.1")); // reserved 240/4
575 + assert!(!validate_url("http://255.255.255.254"));
576 + }
577 +
578 + #[test]
579 + fn is_private_ip_blocks_v6_embedded_v4() {
580 + use std::net::{IpAddr, Ipv6Addr};
581 + // v4-mapped private
582 + assert!(is_private_ip(IpAddr::V6("::ffff:10.0.0.1".parse::<Ipv6Addr>().unwrap())));
583 + // v4-mapped public
584 + assert!(!is_private_ip(IpAddr::V6("::ffff:8.8.8.8".parse::<Ipv6Addr>().unwrap())));
585 + // NAT64 wrapping link-local metadata
586 + assert!(is_private_ip(IpAddr::V6("64:ff9b::169.254.169.254".parse::<Ipv6Addr>().unwrap())));
587 + }
588 +
503 589 #[tokio::test]
504 590 async fn noop_fetcher_returns_none_without_network() {
505 591 let fetcher = LinkPreviewFetcher::Noop;
M multithreaded/src/routes/account.rs +7 -22
@@ -12,13 +12,13 @@
12 12 };
13 13 use tower_sessions::Session;
14 14
15 - use crate::auth::MaybeUser;
15 + use crate::auth::RequireUser;
16 16 use crate::csrf;
17 17 use crate::templates::*;
18 18 use crate::AppState;
19 19
20 20 use super::{
21 - field_error, render_markdown, render_markdown_plus, template_user, SignatureForm,
21 + db_error, field_error, render_markdown, render_markdown_plus, template_user, SignatureForm,
22 22 };
23 23
24 24 const SIGNATURE_MAX: usize = 1024;
@@ -27,19 +27,13 @@
27 27 pub(super) async fn account_settings(
28 28 State(state): State<AppState>,
29 29 session: Session,
30 - MaybeUser(session_user): MaybeUser,
30 + RequireUser(user): RequireUser,
31 31 ) -> Result<AccountSettingsTemplate, Response> {
32 32 let csrf_token = Some(csrf::get_or_create_token(&session).await);
33 - let user = session_user
34 - .ok_or_else(|| Redirect::to("/auth/login").into_response())?;
35 -
36 33 let (signature_markdown, signature_html) =
37 34 mt_db::queries::get_user_signature(&state.db, user.user_id)
38 35 .await
39 - .map_err(|e| {
40 - tracing::error!(error = ?e, "db error loading signature");
41 - StatusCode::INTERNAL_SERVER_ERROR.into_response()
42 - })?
36 + .map_err(db_error)?
43 37 .unwrap_or((None, None));
44 38
45 39 Ok(AccountSettingsTemplate {
@@ -56,12 +50,9 @@
56 50 #[tracing::instrument(skip_all)]
57 51 pub(super) async fn update_signature_handler(
58 52 State(state): State<AppState>,
59 - MaybeUser(session_user): MaybeUser,
53 + RequireUser(user): RequireUser,
60 54 Form(form): Form<SignatureForm>,
61 55 ) -> Result<Redirect, Response> {
62 - let user = session_user
63 - .ok_or_else(|| Redirect::to("/auth/login").into_response())?;
64 -
65 56 if !user.perks.effective_plus() {
66 57 return Err((
67 58 StatusCode::FORBIDDEN,
@@ -75,10 +66,7 @@
75 66 if form.clear.as_deref() == Some("1") {
76 67 mt_db::mutations::clear_user_signature(&state.db, user.user_id)
77 68 .await
78 - .map_err(|e| {
79 - tracing::error!(error = ?e, "db error clearing signature");
80 - StatusCode::INTERNAL_SERVER_ERROR.into_response()
81 - })?;
69 + .map_err(db_error)?;
82 70 return Ok(Redirect::to("/account?toast=Signature+cleared"));
83 71 }
84 72
@@ -108,10 +96,7 @@
108 96
109 97 mt_db::mutations::set_user_signature(&state.db, user.user_id, trimmed, &signature_html)
110 98 .await
111 - .map_err(|e| {
112 - tracing::error!(error = ?e, "db error saving signature");
113 - StatusCode::INTERNAL_SERVER_ERROR.into_response()
114 - })?;
99 + .map_err(db_error)?;
115 100
116 101 Ok(Redirect::to("/account?toast=Signature+saved"))
117 102 }
M multithreaded/src/routes/admin.rs +3 -9
@@ -16,7 +16,7 @@
16 16 use mt_core::types::{ModAction, ModActor};
17 17
18 18 use super::{
19 - audit, begin_tx, commit_tx, get_community, parse_uuid, template_user, AdminSearchQuery,
19 + audit, begin_tx, commit_tx, db_error, get_community, parse_uuid, template_user, AdminSearchQuery,
20 20 CleanSlateForm, SuspendForm,
21 21 };
22 22
@@ -203,17 +203,11 @@
203 203
204 204 let thread_count = mt_db::queries::count_threads_in_community(&state.db, community.id)
205 205 .await
206 - .map_err(|e| {
207 - tracing::error!(error = ?e, "db error counting threads");
208 - StatusCode::INTERNAL_SERVER_ERROR.into_response()
209 - })?;
206 + .map_err(db_error)?;
210 207
211 208 let member_count = mt_db::queries::count_community_members(&state.db, community.id)
212 209 .await
213 - .map_err(|e| {
214 - tracing::error!(error = ?e, "db error counting members");
215 - StatusCode::INTERNAL_SERVER_ERROR.into_response()
216 - })?;
210 + .map_err(db_error)?;
217 211
218 212 let suspension_reason: Option<String> = if community.suspended_at.is_some() {
219 213 mt_db::queries::get_community_suspension_reason(&state.db, community.id)
M multithreaded/src/routes/flagging.rs +4 -13
M multithreaded/src/routes/moderation.rs +22 -82
M multithreaded/src/routes/scope.rs +2 -5
M multithreaded/src/routes/search.rs +2 -5
M multithreaded/src/routes/settings.rs +26 -87
M multithreaded/src/routes/tracking.rs +23 -43
M multithreaded/src/routes/uploads.rs +7 -25
M multithreaded/src/templates/mod.rs -2
M multithreaded/templates/pages/category.html +1 -11
M multithreaded/templates/partials/site_header.html +1 -2
M multithreaded/tests/workflows/auth.rs +18 -5
M multithreaded/tests/workflows/flagging.rs +93
M multithreaded/tests/workflows/moderation.rs +29 -1
M multithreaded/tests/workflows/tracking.rs +55
M multithreaded/crates/mt-db/src/queries.rs +10 -3
M multithreaded/src/routes/forum/actions.rs +12 -39
M multithreaded/src/routes/forum/posts.rs +19 -70
M multithreaded/src/routes/forum/thread.rs +7 -22
M multithreaded/src/routes/forum/views.rs +31 -54
M multithreaded/src/routes/helpers/mod.rs +11
R multithreaded/.sqlx/query-15333e4a76a069b3279777bae4abbe6c1b57158c3c5a2e29350972d879bb177d.json → multithreaded/.sqlx/query-4151f8cee14608e833ae57a2bc89dee8f084ab963a0e145257f41b42715f50e5.json +2 -2
R multithreaded/.sqlx/query-6ca606fbba7c525e0e48b472381eaf3a4660092155db3abf821c5a45e01ec9b0.json → multithreaded/.sqlx/query-2a4cff2675d7b7922a83ddb16555ef7a980541cfbbaea6e399c4e241d3457863.json +3 -2
A multithreaded/migrations/033_drop_guest_role.sql +10
D multithreaded/src/templates/partials.rs -6