Skip to main content

max / makenotwork

audit Run 15 Phase 2: security fixes - /health HTML dashboard is admin-only: anonymous/non-admin callers get a minimal reachable status page (HTTP 200, no metrics) so uptime monitors stay green, while the full dashboard (S3 bucket/region, Stripe mode, pool stats, business counts) requires an admin session. - SSH key fingerprints now have a global UNIQUE (migration 161, dedup-first): the same public key can't be registered to two accounts, closing the duplicate-row CLI-auth denial-of-service. add_key returns a clear cross-account message; the identity lookup gains ORDER BY + LIMIT 1. - Inbound patch handler rejects suspended senders (parity with the issue handler), so a verified-but-suspended user can't post patches. - get_public_project_by_slug excludes sandbox-owned projects, hiding their /p/{slug}, RSS, and blog pages from public view (matches item/user pages); personal_feed gains the same sandbox guard as its sibling feeds. - Custom promo codes are charset-restricted to [A-Z0-9_-] at creation and escaped at the redemptions-modal JS sink (defense-in-depth self-XSS fix). - CSV import rejects emails whose local part starts with a spreadsheet formula trigger (=+-@), closing the re-export formula-injection vector without corrupting sendable addresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-02 00:13 UTC
Commit: bf61ca77c6e4eb8da9f6d0fbc108e16ad69a5b3f
Parent: 00144af
11 files changed, +120 insertions, -27 deletions
A server/migrations/161_ssh_key_global_fingerprint_unique.sql +18
@@ -0,0 +1,18 @@
1 + -- An SSH key fingerprint must map to exactly one identity. The original table
2 + -- only enforced UNIQUE (user_id, fingerprint), so the same public key could be
3 + -- registered to multiple accounts. Because SSH public keys are non-secret (they
4 + -- are published, and MNW itself renders them into authorized_keys), an attacker
5 + -- could register a victim's key on their own account; the CLI SSH identity
6 + -- lookup then returns >1 row and fails closed, breaking the victim's CLI auth
7 + -- (Run 15 Auth C1). Add a global UNIQUE (fingerprint).
8 +
9 + -- Dedup first: if any fingerprint is registered to more than one account, keep
10 + -- the earliest registration (the legitimate owner; a cross-account duplicate is
11 + -- always added later) and drop the rest, so the constraint can be added.
12 + DELETE FROM ssh_keys a
13 + USING ssh_keys b
14 + WHERE a.fingerprint = b.fingerprint
15 + AND (a.created_at, a.id) > (b.created_at, b.id);
16 +
17 + ALTER TABLE ssh_keys
18 + ADD CONSTRAINT ssh_keys_fingerprint_key UNIQUE (fingerprint);
M server/src/db/projects.rs +8 -2
@@ -320,14 +320,20 @@ pub async fn get_public_projects_with_item_counts(
320 320 Ok(projects)
321 321 }
322 322
323 - /// Fetch a public project by its URL slug. Returns `None` if not found or not public.
323 + /// Fetch a public project by its URL slug. Returns `None` if not found, not
324 + /// public, or owned by a sandbox account. Sandbox accounts are hidden from all
325 + /// public surfaces (discover, item pages, user pages); this join closes the gap
326 + /// where their `/p/{slug}`, RSS feeds, and blog pages still rendered publicly.
324 327 #[tracing::instrument(skip_all)]
325 328 pub async fn get_public_project_by_slug(
326 329 pool: &PgPool,
327 330 slug: &Slug,
328 331 ) -> Result<Option<DbProject>> {
329 332 let project = sqlx::query_as::<_, DbProject>(
330 - "SELECT * FROM projects WHERE slug = $1 AND is_public = true ORDER BY created_at ASC LIMIT 1",
333 + "SELECT p.* FROM projects p \
334 + JOIN users u ON u.id = p.user_id \
335 + WHERE p.slug = $1 AND p.is_public = true AND u.is_sandbox = false \
336 + ORDER BY p.created_at ASC LIMIT 1",
331 337 )
332 338 .bind(slug)
333 339 .fetch_optional(pool)
M server/src/db/ssh_keys.rs +2
@@ -107,6 +107,8 @@ pub async fn lookup_user_by_fingerprint(
107 107 FROM ssh_keys sk
108 108 JOIN users u ON u.id = sk.user_id
109 109 WHERE sk.fingerprint = $1
110 + ORDER BY sk.created_at ASC
111 + LIMIT 1
110 112 "#,
111 113 )
112 114 .bind(fingerprint)
M server/src/import/csv_converter.rs +7
@@ -55,6 +55,13 @@ pub fn parse_csv(bytes: &[u8], mapping: &ColumnMapping) -> Result<ImportPayload>
55 55 let parts: Vec<&str> = s.splitn(2, '@').collect();
56 56 parts.len() == 2
57 57 && !parts[0].is_empty()
58 + // Reject local parts starting with a spreadsheet formula
59 + // trigger (=, +, -, @). Such addresses aren't valid anyway,
60 + // and rejecting them keeps the stored email — which is what a
61 + // later CSV export re-emits — from carrying a live formula
62 + // (the email column is used for sending, so it can't be
63 + // quote-prefixed like the other sanitized fields).
64 + && !parts[0].starts_with(['=', '+', '-', '@'])
58 65 && parts[1].contains('.')
59 66 && !parts[1].starts_with('.')
60 67 && !parts[1].ends_with('.')
M server/src/routes/api/promo_codes.rs +9
@@ -96,6 +96,15 @@ pub(super) async fn create_promo_code(
96 96 if code.is_empty() || code.len() > 50 {
97 97 return Err(AppError::BadRequest("Code must be 1-50 characters".to_string()));
98 98 }
99 + // Restrict to an unambiguous coupon-code charset. Besides being what
100 + // users expect to type, this keeps HTML metacharacters out of the
101 + // code so it can never carry markup into the redemptions modal
102 + // (defense-in-depth alongside escaping at the JS sink).
103 + if !code.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') {
104 + return Err(AppError::BadRequest(
105 + "Code may only contain letters, numbers, hyphens, and underscores".to_string(),
106 + ));
107 + }
99 108 code
100 109 }
101 110 };
M server/src/routes/api/ssh_keys.rs +15 -8
@@ -81,14 +81,21 @@ pub(super) async fn add_key(
81 81 .await
82 82 .map_err(|e| {
83 83 // Check for unique constraint violation (duplicate fingerprint)
84 - if let AppError::Database(ref db_err) = e
85 - && db_err
86 - .to_string()
87 - .contains("ssh_keys_user_id_fingerprint_key")
88 - {
89 - return AppError::validation(
90 - "This SSH key is already registered to your account".to_string(),
91 - );
84 + if let AppError::Database(ref db_err) = e {
85 + let msg = db_err.to_string();
86 + if msg.contains("ssh_keys_user_id_fingerprint_key") {
87 + return AppError::validation(
88 + "This SSH key is already registered to your account".to_string(),
89 + );
90 + }
91 + // Global uniqueness: the key is registered to a different account.
92 + // A fingerprint maps to exactly one identity, so we reject rather
93 + // than let a duplicate break the owner's CLI SSH auth.
94 + if msg.contains("ssh_keys_fingerprint_key") {
95 + return AppError::validation(
96 + "This SSH key is already registered to another account.".to_string(),
97 + );
98 + }
92 99 }
93 100 e
94 101 })?;
M server/src/routes/pages/feeds.rs +6
@@ -277,6 +277,12 @@ async fn personal_feed(
277 277 .await?
278 278 .ok_or(AppError::NotFound)?;
279 279
280 + // Sandbox accounts are hidden from every public feed surface (the other four
281 + // feed handlers reject them); keep this one consistent.
282 + if db_user.is_sandbox {
283 + return Err(AppError::NotFound);
284 + }
285 +
280 286 // Enforce key rotation: a signature valid for a stale version is a revoked
281 287 // URL (the user hit "Regenerate feed URL"). The HMAC above proves the `v`
282 288 // wasn't forged; this proves it's still current.
M server/src/routes/pages/public/health/mod.rs +25 -2
@@ -508,12 +508,34 @@ async fn collect_health(state: &AppState) -> HealthData {
508 508 }
509 509
510 510 /// Render the system health dashboard with database, storage, and service status.
511 + ///
512 + /// The detailed dashboard — infra config, DB pool internals, business counts,
513 + /// Stripe mode, S3 bucket/region — is admin-only. Anonymous and non-admin callers
514 + /// get a minimal, reachable status page (HTTP 200) so uptime monitors (PoM polls
515 + /// this URL) stay green without the operational-intel disclosure. `/api/health`
516 + /// remains the minimal JSON probe.
511 517 #[tracing::instrument(skip_all, name = "health::health")]
512 518 pub(super) async fn health(
513 519 State(state): State<AppState>,
514 520 session: Session,
515 - ) -> Result<impl IntoResponse> {
521 + crate::auth::MaybeUserVerified(maybe_user): crate::auth::MaybeUserVerified,
522 + ) -> Result<axum::response::Response> {
516 523 let data = collect_health(&state).await;
524 +
525 + let is_admin = maybe_user.as_ref().is_some_and(|u| u.is_admin);
526 + if !is_admin {
527 + // `label()` is a controlled &'static str from OverallStatus — no user input.
528 + let body = axum::response::Html(format!(
529 + "<!doctype html><html lang=\"en\"><head><meta charset=\"utf-8\">\
530 + <meta name=\"viewport\" content=\"width=device-width, initial-scale=1\">\
531 + <title>MakeNotWork status</title></head>\
532 + <body style=\"font-family:system-ui,sans-serif;max-width:32rem;margin:4rem auto;padding:0 1rem\">\
533 + <h1>MakeNotWork</h1><p>{}</p></body></html>",
534 + data.overall.label()
535 + ));
536 + return Ok(body.into_response());
537 + }
538 +
517 539 let now = chrono::Utc::now();
518 540
519 541 let pool_utilization = if data.db_pool_max > 0 {
@@ -623,7 +645,8 @@ pub(super) async fn health(
623 645 pom_routes_ok: data.pom_routes_ok,
624 646 pom_routes_failed: data.pom_routes_failed,
625 647 privacy_jobs: format_privacy_jobs(&data.privacy_jobs, now),
626 - })
648 + }
649 + .into_response())
627 650 }
628 651
629 652 // ============================================================================
M server/src/routes/postmark/patches.rs +5 -1
@@ -78,7 +78,11 @@ pub(super) async fn postmark_inbound(
78 78 return HandlerOutcome::Terminal(StatusCode::OK);
79 79 }
80 80 let sender = match db::users::get_user_by_email(&state.db, &sender_email).await {
81 - Ok(Some(u)) if u.email_verified => u,
81 + Ok(Some(u)) if u.email_verified && !u.is_suspended() => u,
82 + Ok(Some(u)) if u.is_suspended() => {
83 + tracing::info!(email = %sender_email, "inbound: sender is suspended");
84 + return HandlerOutcome::Terminal(StatusCode::OK);
85 + }
82 86 Ok(Some(_)) => {
83 87 tracing::info!(email = %sender_email, "inbound: sender email not verified");
84 88 return HandlerOutcome::Terminal(StatusCode::OK);
M server/static/tab-project-promotions-2.js +1 -1
@@ -39,7 +39,7 @@ function showPromoRedemptions(codeId, codeLabel) {
39 39 content.style.padding = '2rem';
40 40 content.innerHTML =
41 41 '<div class="modal-header" style="margin-bottom: 1rem;">'
42 - + '<h2>Redemptions: <code>' + codeLabel + '</code></h2>'
42 + + '<h2>Redemptions: <code>' + escapeHtml(codeLabel) + '</code></h2>'
43 43 + '<button type="button" class="modal-close" aria-label="Dismiss"'
44 44 + ' onclick="document.getElementById(\'promo-redemptions-modal\').remove()">&times;</button>'
45 45 + '</div>'
M server/tests/workflows/promo_codes_discount.rs +24 -13
@@ -272,33 +272,44 @@ async fn discount_code_project_scoped() {
272 272 "Project-scoped code should appear in listing");
273 273 }
274 274
275 - // UX-1: free-text rendered into a JS-string context (inline onclick) must be
276 - // escaped. A promo code is only length-validated, so it can contain a quote; it
277 - // must land in an escaped data-* attribute with the copy handler reading dataset,
278 - // never interpolated raw into a writeText('...') literal (which a quote breaks).
275 + // UX-1: a promo code must never reach a JS-string context unescaped. Two layers
276 + // now enforce this: (1) custom codes are charset-restricted at creation to
277 + // [A-Z0-9_-], so a quote or HTML metacharacter can't enter in the first place;
278 + // (2) the code still renders into an escaped data-* attribute read by the copy
279 + // handler, never interpolated raw into a writeText('...') literal.
279 280 #[tokio::test]
280 - async fn promo_code_with_quote_is_not_in_js_string_context() {
281 + async fn promo_code_with_quote_is_rejected_and_valid_code_is_not_in_js_string_context() {
281 282 let mut h = TestHarness::new().await;
282 283 let setup = h.create_creator_with_item("xssseller", "digital", 1000).await;
283 284 h.publish_project_and_item(&setup.project_id, &setup.item_id).await;
284 285
285 - // An HTMX create returns the rendered promo_codes_list.html partial (otherwise
286 - // the route returns JSON); htmx_post_form also injects the CSRF token.
287 - let resp = h
286 + // Layer 1: a code containing a quote is rejected by the charset validator.
287 + let rejected = h
288 288 .client
289 289 .htmx_post_form(
290 290 "/api/promo-codes",
291 291 "code=AB'CD&code_purpose=discount&discount_type=percentage&discount_value=25",
292 292 )
293 293 .await;
294 + assert!(
295 + rejected.status.is_client_error(),
296 + "quoted code must be rejected, got {}: {}",
297 + rejected.status, rejected.text
298 + );
299 +
300 + // Layer 2: a valid code still lands in an escaped data-* attribute, and the
301 + // copy handler reads dataset rather than interpolating the code.
302 + let resp = h
303 + .client
304 + .htmx_post_form(
305 + "/api/promo-codes",
306 + "code=AB-CD&code_purpose=discount&discount_type=percentage&discount_value=25",
307 + )
308 + .await;
294 309 assert!(resp.status.is_success(), "create failed: {} {}", resp.status, resp.text);
295 310
296 311 let html = &resp.text;
297 - // Code is uppercased to AB'CD. The raw quote must never appear unescaped
298 - // (Askama escapes it in both the cell text and the data attribute).
299 - assert!(!html.contains("AB'CD"), "raw quoted code must not appear unescaped: {html}");
300 - // The copy handler reads the data attribute instead of interpolating the code.
301 312 assert!(html.contains("writeText(this.dataset.code)"), "copy must read dataset: {html}");
302 313 assert!(!html.contains("writeText('"), "code must not be interpolated into a JS string literal");
303 - assert!(html.contains("data-code="), "escaped code must live in a data-code attr: {html}");
314 + assert!(html.contains("data-code="), "code must live in a data-code attr: {html}");
304 315 }