max / makenotwork

server: mt audit findings — health 503, SSRF img-proxy e2e, hardening Audit 2026-07-02 findings (all gate-green: clippy -D warnings, 437 tests): - /api/health returns 503 when the DB is unreachable (was always 200), so status-only probes can't read a degraded box as healthy; health_status test. - Add SSRF/img-proxy end-to-end tests driving the real handler with the production client wired (HarnessOptions::link_preview_http): login gate + 502 refusal of metadata-IP/loopback/RFC1918/bad-port/bad-scheme. - Link-preview client gets a client-level timeout backstop (10s/5s). - assert_secure_url now guards OAUTH_REDIRECT_URI too, not just MNW_BASE_URL. - CSP adds object-src 'none'; base-uri 'none'; form-action 'self'. - mt.js wrapped in one IIFE (no leaked globals); dead csrfHeaders removed. - seed.rs renders through docengine (matches production, drops hand-rolled escape). - search_handler documents its intentional public-read posture. - docs/architecture.md: migration CONCURRENTLY/IF NOT EXISTS conventions; corrected the stale connection-pool paragraph. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: Max Johnson <me@maxj.phd> · 2026-07-02 22:01 UTC

Commit: 37d5fdcbddba45fc5686801d0af8e427773de842

Parent: 030b70c

11 files changed, +208 insertions, -35 deletions

M multithreaded/docs/architecture.md +11 -1

			@@ -185,4 +185,14 @@ Images are stored in S3 (not in PostgreSQL), with metadata tracked in the databa
185	185
186	186		### Connection pooling
187	187
188		-	MT uses sqlx's built-in connection pool (`PgPool`). Sessions, forum data, and search all share the same pool. The session store has its own cleanup task but no separate connection pool.
	188	+	MT uses sqlx's built-in connection pool (`PgPool`). Forum data and search share the handler pool (32 connections, sized for ~10 concurrent `try_join!` fan-outs). The Postgres session store runs on its own dedicated 6-connection pool so session I/O and the hourly expiry sweep don't contend with handler queries.
	189	+
	190	+	### Migrations
	191	+
	192	+	Migrations live in `migrations/` and run forward-only via `sqlx::migrate!` at boot; sqlx records a checksum per applied migration, so an already-applied migration file must never be edited (a changed checksum aborts boot on every existing deployment). Fix mistakes with a new migration.
	193	+
	194	+	Conventions for new migrations:
	195	+
	196	+	- Index creation on populated tables must use `CREATE INDEX CONCURRENTLY`. A plain `CREATE INDEX` takes an `ACCESS EXCLUSIVE` lock and blocks writes for the duration of the build. `CONCURRENTLY` cannot run inside a transaction, so such a migration must contain only that one statement. The historical index migrations (018 GIN full-text/trigram, 023/024/029 on `posts`/`threads`) were applied pre-population and predate this rule; they are safe as-is but are not the pattern to copy.
	197	+	- Use `IF NOT EXISTS` / `IF EXISTS` on `CREATE`/`DROP` so a partially-applied set can be re-run. Later migrations (024+) follow this; some early ones (005/006/008/009/011-019) do not — again, do not retro-edit, just follow the convention going forward.
	198	+	- Prefer additive changes. Destructive changes (`DROP COLUMN`, `ALTER COLUMN ... DROP NOT NULL`) must carry a comment explaining why they are safe (see 027, 032).

M multithreaded/src/config.rs +13 -10

			@@ -50,13 +50,15 @@ impl Config {
50	50		pub fn from_env() -> Self {
51	51		let mnw_base_url =
52	52		std::env::var("MNW_BASE_URL").unwrap_or_else(\|_\| "http://127.0.0.1:3000".to_string());
53		-	assert_secure_base_url(&mnw_base_url);
	53	+	assert_secure_url("MNW_BASE_URL", &mnw_base_url);
	54	+	let oauth_redirect_uri = std::env::var("OAUTH_REDIRECT_URI")
	55	+	.unwrap_or_else(\|_\| "http://127.0.0.1:3400/auth/callback".to_string());
	56	+	assert_secure_url("OAUTH_REDIRECT_URI", &oauth_redirect_uri);
54	57		Self {
55	58		mnw_base_url: mnw_base_url.into(),
56	59		oauth_client_id: std::env::var("OAUTH_CLIENT_ID")
57	60		.expect("OAUTH_CLIENT_ID must be set"),
58		-	oauth_redirect_uri: std::env::var("OAUTH_REDIRECT_URI")
59		-	.unwrap_or_else(\|_\| "http://127.0.0.1:3400/auth/callback".to_string()),
	61	+	oauth_redirect_uri,
60	62		platform_admin_id: std::env::var("PLATFORM_ADMIN_ID")
61	63		.ok()
62	64		.and_then(\|s\| Uuid::parse_str(&s).ok()),
			@@ -70,18 +72,19 @@ impl Config {
70	72		}
71	73		}
72	74
73		-	/// Refuse to boot a real deployment with a non-`https` base URL.
	75	+	/// Refuse to boot a real deployment with a non-`https` URL.
74	76		///
75		-	/// A non-loopback `MNW_BASE_URL` served over plain `http` ships OAuth redirects,
76		-	/// session cookies, and absolute links over cleartext. Loopback hosts
77		-	/// (`127.0.0.1`, `localhost`, `[::1]`) are exempt so local HTTP development still
78		-	/// works; anything else must be `https`.
79		-	fn assert_secure_base_url(url: &str) {
	77	+	/// A non-loopback URL served over plain `http` ships OAuth redirects (which carry
	78	+	/// the auth code), session cookies, and absolute links over cleartext. Applies to
	79	+	/// both `MNW_BASE_URL` and `OAUTH_REDIRECT_URI`. Loopback hosts (`127.0.0.1`,
	80	+	/// `localhost`, `[::1]`) are exempt so local HTTP development still works;
	81	+	/// anything else must be `https`.
	82	+	fn assert_secure_url(var_name: &str, url: &str) {
80	83		let is_loopback =
81	84		url.contains("127.0.0.1") \|\| url.contains("localhost") \|\| url.contains("[::1]");
82	85		assert!(
83	86		is_loopback \|\| url.starts_with("https://"),
84		-	"MNW_BASE_URL must be https for a non-loopback deployment (got `{url}`)"
	87	+	"{var_name} must be https for a non-loopback deployment (got `{url}`)"
85	88		);
86	89		}
87	90

M multithreaded/src/link_preview.rs +5

			@@ -143,6 +143,11 @@ impl reqwest::dns::Resolve for SsrfSafeResolver {
143	143		/// policy and a connect-time resolver that refuses private addresses.
144	144		pub fn build_preview_client() -> reqwest::Client {
145	145		reqwest::Client::builder()
	146	+	// Client-level backstop so every call site is bounded even if a
	147	+	// per-request `.timeout()` is omitted (call sites still set a tighter
	148	+	// 5s; this only catches a future caller that forgets).
	149	+	.timeout(std::time::Duration::from_secs(10))
	150	+	.connect_timeout(std::time::Duration::from_secs(5))
146	151		.dns_resolver(std::sync::Arc::new(SsrfSafeResolver))
147	152		.redirect(reqwest::redirect::Policy::custom(\|attempt\| {
148	153		if !validate_url(attempt.url().as_str()) \|\| attempt.previous().len() >= 5 {

M multithreaded/src/main.rs +3 -1

			@@ -142,7 +142,9 @@ async fn main() {
142	142		.layer(tower_http::set_header::SetResponseHeaderLayer::overriding(
143	143		axum::http::header::CONTENT_SECURITY_POLICY,
144	144		axum::http::HeaderValue::from_static(
145		-	"default-src 'self'; img-src 'self'; style-src 'self'; frame-ancestors 'none'",
	145	+	"default-src 'self'; img-src 'self'; style-src 'self'; \
	146	+	frame-ancestors 'none'; object-src 'none'; base-uri 'none'; \
	147	+	form-action 'self'",
146	148		),
147	149		))
148	150		.layer(tower_http::set_header::SetResponseHeaderLayer::overriding(

M multithreaded/src/routes/mod.rs +29 -4

			@@ -4,7 +4,7 @@ mod account;
4	4		mod admin;
5	5		mod flagging;
6	6		mod forum;
7		-	mod helpers;
	7	+	pub(crate) mod helpers;
8	8		pub mod internal;
9	9		mod moderation;
10	10		mod scope;
			@@ -393,16 +393,32 @@ pub(super) struct DeleteTagForm {
393	393		// ============================================================================
394	394
395	395		/// Health check — proves the service is responding and the database is reachable.
	396	+	///
	397	+	/// Returns `200 OK` when the DB is reachable and `503 Service Unavailable` when
	398	+	/// it is not, so a status-only uptime probe or load-balancer healthcheck can't
	399	+	/// read a box that can't serve a single DB-backed page as healthy. PoM parses
	400	+	/// the JSON body key-by-key and additionally expects `200` for the operational
	401	+	/// case (`pom/deploy/pom-hetzner.toml`), which the OK branch satisfies.
396	402		#[tracing::instrument(skip_all)]
397	403		async fn health(
398	404		axum::extract::State(state): axum::extract::State<AppState>,
399		-	) -> Json<serde_json::Value> {
	405	+	) -> impl IntoResponse {
400	406		let db_ok = sqlx::query_scalar::<_, i32>("SELECT 1")
401	407		.fetch_one(&state.db)
402	408		.await
403	409		.is_ok();
404	410
405		-	Json(health_body(db_ok))
	411	+	(health_status(db_ok), Json(health_body(db_ok)))
	412	+	}
	413	+
	414	+	/// Map DB reachability to the HTTP status. Pure so the status contract can be
	415	+	/// tested without a live DB (mirrors `health_body`).
	416	+	fn health_status(db_ok: bool) -> StatusCode {
	417	+	if db_ok {
	418	+	StatusCode::OK
	419	+	} else {
	420	+	StatusCode::SERVICE_UNAVAILABLE
	421	+	}
406	422		}
407	423
408	424		/// Build the JSON body for the `/api/health` response.
			@@ -444,7 +460,8 @@ async fn not_found_handler(
444	460
445	461		#[cfg(test)]
446	462		mod health_tests {
447		-	use super::health_body;
	463	+	use super::{health_body, health_status};
	464	+	use axum::http::StatusCode;
448	465
449	466		/// Schema-drift guard for the `mt` target. See `shared/pom-contract/`.
450	467		#[test]
			@@ -456,4 +473,12 @@ mod health_tests {
456	473		&body,
457	474		);
458	475		}
	476	+
	477	+	/// A reachable DB is `200`; an unreachable DB is `503` so status-only probes
	478	+	/// don't read a degraded box as healthy. PoM expects `200` for operational.
	479	+	#[test]
	480	+	fn health_status_reflects_db_reachability() {
	481	+	assert_eq!(health_status(true), StatusCode::OK);
	482	+	assert_eq!(health_status(false), StatusCode::SERVICE_UNAVAILABLE);
	483	+	}
459	484		}

M multithreaded/src/routes/search.rs +11

			@@ -17,6 +17,17 @@ pub(super) struct SearchQuery {
17	17		}
18	18
19	19		/// GET /search?q=...&scope=... — returns HTML fragment for HTMX swap.
	20	+	///
	21	+	/// Access posture: intentionally public. There is no members-only or private
	22	+	/// community — every community is public-read (the `state` machine only gates
	23	+	/// writes: restricted/frozen/archived, migration 025), so an anonymous request
	24	+	/// already sees all the content search can surface. A community ban restricts
	25	+	/// participation, not reading, so there is no additional content to withhold from
	26	+	/// a banned viewer that they couldn't reach anonymously. Content that must never
	27	+	/// surface is excluded in the query itself (`queries::search_threads` filters
	28	+	/// `deleted_at`, `suspended_at`, and `removed_at`), not per-viewer here. If a
	29	+	/// private-community concept is ever added, this handler must take `MaybeUser`
	30	+	/// and filter by membership/ban — until then, taking no session is correct.
20	31		#[tracing::instrument(skip_all)]
21	32		pub(super) async fn search_handler(
22	33		axum::extract::State(state): axum::extract::State<AppState>,

M multithreaded/src/seed.rs +4 -10

			@@ -990,16 +990,10 @@ async fn seed_post(
990	990		author_id: Uuid,
991	991		body_markdown: &str,
992	992		) -> Uuid {
993		-	let body_html = body_markdown
994		-	.split("\n\n")
995		-	.map(\|p\| {
996		-	let escaped = p
997		-	.replace('&', "&")
998		-	.replace('<', "<")
999		-	.replace('>', ">");
1000		-	format!("<p>{escaped}</p>")
1001		-	})
1002		-	.collect::<String>();
	993	+	// Render through the same strict markdown path production uses so seeded
	994	+	// posts render identically to real ones (previously a hand-rolled escape
	995	+	// diverged from docengine output).
	996	+	let body_html = crate::routes::helpers::render_markdown(body_markdown);
1003	997
1004	998		let post_id: Uuid = sqlx::query_scalar(
1005	999		"INSERT INTO posts (thread_id, author_id, body_markdown, body_html)

M multithreaded/static/mt.js +6 -8

			@@ -1,14 +1,11 @@
1	1		/* Multithreaded — Core JavaScript */
2	2		'use strict';
3	3
4		-	/* ===========================================
5		-	CSRF
6		-	=========================================== */
7		-
8		-	function csrfHeaders() {
9		-	var token = document.querySelector('meta[name="csrf-token"]')?.content;
10		-	return token ? { 'X-CSRF-Token': token } : {};
11		-	}
	4	+	/* Everything below is wrapped in one IIFE so no helper (showToast,
	5	+	showFormError, openSearchModal, ...) leaks onto `window`. Templates carry no
	6	+	inline handlers (the CSP has no 'unsafe-inline'); every binding is via
	7	+	addEventListener, so nothing needs to be a global. */
	8	+	(function () {
12	9
13	10		(function() {
14	11		var csrfToken = document.querySelector('meta[name="csrf-token"]')?.content;
			@@ -585,3 +582,4 @@ document.addEventListener('submit', function(e) {
585	582		}
586	583		});
587	584		})();
	585	+	})();

M multithreaded/tests/harness/mod.rs +11 -1

			@@ -26,6 +26,10 @@ pub struct HarnessOptions {
26	26		/// Override MNW base URL — point at a wiremock server for tests that exercise
27	27		/// OAuth/userinfo flows. Defaults to a black-hole URL that fails fast.
28	28		pub mnw_base_url: Option<String>,
	29	+	/// Wire the real SSRF-safe link-preview HTTP client instead of `Noop`, so
	30	+	/// tests can drive `/img-proxy` through the production fetch path. Off by
	31	+	/// default (post-creation preview fetches stay inert in the common case).
	32	+	pub link_preview_http: bool,
29	33		}
30	34
31	35		impl TestHarness {
			@@ -71,7 +75,13 @@ impl TestHarness {
71	75		db: pool.clone(),
72	76		config,
73	77		http: reqwest::Client::new(),
74		-	link_preview: multithreaded::link_preview::LinkPreviewFetcher::Noop,
	78	+	link_preview: if opts.link_preview_http {
	79	+	multithreaded::link_preview::LinkPreviewFetcher::Http(
	80	+	multithreaded::link_preview::build_preview_client(),
	81	+	)
	82	+	} else {
	83	+	multithreaded::link_preview::LinkPreviewFetcher::Noop
	84	+	},
75	85		s3: None,
76	86		};
77	87

A multithreaded/tests/workflows/img_proxy.rs +114

		@@ -0,0 +1,114 @@
1	+	//! End-to-end coverage for the `/img-proxy` external-image proxy (M-UX1).
2	+	//!
3	+	//! These drive the REAL handler with the production SSRF-safe HTTP client wired
4	+	//! (`link_preview_http`), so they exercise routing -> the login gate -> client
5	+	//! selection -> `fetch_image` -> `validate_url` refusal — not just the unit
6	+	//! `validate_url` string check.
7	+	//!
8	+	//! Scope note: `validate_url` rejects literal private/reserved targets and bad
9	+	//! ports/schemes before any socket is opened, so these cases are fully
10	+	//! hermetic (no network, deterministic). The connect-time `SsrfSafeResolver`
11	+	//! layer — which catches a public hostname that resolves to a private address,
12	+	//! including DNS rebinding — cannot be exercised hermetically: it needs real
13	+	//! DNS, and any loopback mock server is itself refused as private. That layer
14	+	//! stays covered by its own resolver logic plus the `validate_url` unit tests.
15	+
16	+	use crate::harness::{HarnessOptions, TestHarness};
17	+	use axum::http::StatusCode;
18	+
19	+	async fn http_preview_harness() -> TestHarness {
20	+	TestHarness::with_options(HarnessOptions {
21	+	link_preview_http: true,
22	+	..Default::default()
23	+	})
24	+	.await
25	+	}
26	+
27	+	#[tokio::test]
28	+	async fn img_proxy_requires_login() {
29	+	let mut h = http_preview_harness().await;
30	+	// Auth is checked before any fetch, so even a well-formed target is rejected.
31	+	let resp = h.client.get("/img-proxy?u=http://example.com/a.png").await;
32	+	assert_eq!(
33	+	resp.status,
34	+	StatusCode::UNAUTHORIZED,
35	+	"anonymous /img-proxy must be 401, got {}",
36	+	resp.status
37	+	);
38	+	}
39	+
40	+	#[tokio::test]
41	+	async fn img_proxy_refuses_cloud_metadata_ip() {
42	+	let mut h = http_preview_harness().await;
43	+	h.login_as("proxyuser_meta").await;
44	+	let resp = h
45	+	.client
46	+	.get("/img-proxy?u=http://169.254.169.254/latest/meta-data/")
47	+	.await;
48	+	assert_eq!(
49	+	resp.status,
50	+	StatusCode::BAD_GATEWAY,
51	+	"link-local cloud-metadata IP must be refused (502), got {}",
52	+	resp.status
53	+	);
54	+	}
55	+
56	+	#[tokio::test]
57	+	async fn img_proxy_refuses_loopback() {
58	+	let mut h = http_preview_harness().await;
59	+	h.login_as("proxyuser_loop").await;
60	+	let resp = h
61	+	.client
62	+	.get("/img-proxy?u=http://127.0.0.1:1/x.png")
63	+	.await;
64	+	assert_eq!(
65	+	resp.status,
66	+	StatusCode::BAD_GATEWAY,
67	+	"loopback target must be refused (502), got {}",
68	+	resp.status
69	+	);
70	+	}
71	+
72	+	#[tokio::test]
73	+	async fn img_proxy_refuses_rfc1918_private_ip() {
74	+	let mut h = http_preview_harness().await;
75	+	h.login_as("proxyuser_priv").await;
76	+	let resp = h.client.get("/img-proxy?u=http://10.0.0.5/x.png").await;
77	+	assert_eq!(
78	+	resp.status,
79	+	StatusCode::BAD_GATEWAY,
80	+	"RFC1918 private IP must be refused (502), got {}",
81	+	resp.status
82	+	);
83	+	}
84	+
85	+	#[tokio::test]
86	+	async fn img_proxy_refuses_non_web_port() {
87	+	let mut h = http_preview_harness().await;
88	+	h.login_as("proxyuser_port").await;
89	+	// Public IP, but a non-web port (Redis) — the proxy must not be usable as a
90	+	// request-forwarder to arbitrary services.
91	+	let resp = h.client.get("/img-proxy?u=http://1.1.1.1:6379/x").await;
92	+	assert_eq!(
93	+	resp.status,
94	+	StatusCode::BAD_GATEWAY,
95	+	"non-web port must be refused (502), got {}",
96	+	resp.status
97	+	);
98	+	}
99	+
100	+	#[tokio::test]
101	+	async fn img_proxy_refuses_non_http_scheme() {
102	+	let mut h = http_preview_harness().await;
103	+	h.login_as("proxyuser_scheme").await;
104	+	let resp = h
105	+	.client
106	+	.get("/img-proxy?u=ftp://example.com/x.png")
107	+	.await;
108	+	assert_eq!(
109	+	resp.status,
110	+	StatusCode::BAD_GATEWAY,
111	+	"non-http scheme must be refused (502), got {}",
112	+	resp.status
113	+	);
114	+	}

M multithreaded/tests/workflows/mod.rs +1

			@@ -10,6 +10,7 @@ mod csrf;
10	10		mod endorsements;
11	11		mod flagging;
12	12		mod health;
	13	+	mod img_proxy;
13	14		mod internal_api;
14	15		mod link_previews;
15	16		mod mentions;