Skip to main content

max / makenotwork

cleanups: drop the last db .expect, fix csrf phantom race comment, instrument custom-page write (audit Run 20 Phase 5) - creator_tiers: replace `effective_tier.expect(...)` with a let-else that surfaces AppError::Internal — the last production .expect in the db layer. The path is unreachable (every None returns earlier), but a 500 beats a panic if that invariant ever changes. - csrf::get_or_create_token: remove the post-insert re-read and correct its comment. Two concurrent first-requests each hold an independent session load, so the re-read only ever saw this request's own insert and returned the candidate unconditionally — a no-op. The store reconciles tabs last-writer-wins at save; a losing tab hits one 403 and retries. Behavior is unchanged. - users::update_user_custom_page: add #[instrument]; it mutates user-supplied HTML/CSS and had no span. Migrations 019/117 DROP TABLE left as-is: adding IF EXISTS would change the applied-migration checksum and break sqlx::migrate! on boot, and the drops are safe on fresh replay (tables created earlier in 001/098). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-03 07:43 UTC
Commit: 9da0537a73d204e7a2419affb70ac8a9dccc4080
Parent: d7432e5
3 files changed, +23 insertions, -19 deletions
M server/src/csrf.rs +14 -18
@@ -92,14 +92,12 @@ pub fn generate_token() -> String {
92 92 ///
93 93 /// `tower-sessions`' `insert` is last-write-wins, so two concurrent first
94 94 /// requests (e.g. the user opens two tabs while not yet having a token)
95 - /// can each generate a fresh token and clobber each other — the first
96 - /// form to post then fails with a 403 because its rendered token has
97 - /// already been overwritten.
98 - ///
99 - /// Re-check via `get` after insert: if a different token landed between
100 - /// our get and our insert, prefer THAT value over ours so all renders
101 - /// from this point forward agree. `String` is `Clone`-cheap, and the
102 - /// race window is small enough that the duplicate insert is negligible.
95 + /// can each generate a fresh token and clobber each other at store-save time —
96 + /// the losing tab's rendered token is then stale and its first mutation gets a
97 + /// 403, after which a reload picks up the winning token. This is a rare,
98 + /// self-correcting edge for the brief window before a session has any token; it
99 + /// cannot be reconciled in-process because each request holds an independent
100 + /// session load (see the note in the body).
103 101 pub async fn get_or_create_token(session: &Session) -> Result<String, AppError> {
104 102 if let Some(token) = session
105 103 .get::<String>(CSRF_SESSION_KEY)
@@ -115,16 +113,14 @@ pub async fn get_or_create_token(session: &Session) -> Result<String, AppError>
115 113 .await
116 114 .context("session insert")?;
117 115
118 - // Re-fetch so a concurrent insert wins consistently — whichever caller
119 - // wrote last is what every subsequent render will see, and we hand
120 - // back the same value here.
121 - let final_token: String = session
122 - .get(CSRF_SESSION_KEY)
123 - .await
124 - .context("session error")?
125 - .unwrap_or(candidate);
126 -
127 - Ok(final_token)
116 + // Return the token we just inserted. (A prior version re-read the key here,
117 + // claiming to reconcile a concurrent insert — but two concurrent
118 + // first-requests sharing a cookie each hold an independent session load, so
119 + // the re-read only ever observes THIS request's own insert and returned
120 + // `candidate` unconditionally. The store reconciles the tabs last-writer-wins
121 + // at save time; a losing tab may hit one 403 and retry. The re-read was a
122 + // no-op, so it's gone.)
123 + Ok(candidate)
128 124 }
129 125
130 126 /// Validate a CSRF token against the session token
M server/src/db/creator_tiers.rs +8 -1
@@ -825,7 +825,14 @@ pub async fn check_presign_allowed(
825 825 ));
826 826 }
827 827
828 - let tier = effective_tier.expect("guarded by is_none check above");
828 + let Some(tier) = effective_tier else {
829 + // Unreachable: every None path above returns early. Surface as a 500
830 + // rather than panic if that invariant ever changes (Run 20 — last
831 + // production .expect in the db layer).
832 + return Err(AppError::Internal(anyhow::anyhow!(
833 + "effective_tier unexpectedly None after guard"
834 + )));
835 + };
829 836 if !tier.allows_file_uploads() {
830 837 return Err(AppError::BadRequest(
831 838 "Basic tier is text-only. Upgrade to Small Files or higher to upload files.".to_string(),
M server/src/db/users.rs +1
@@ -104,6 +104,7 @@ pub async fn update_user_profile(
104 104 /// Store a user's custom profile-page source (the original, pre-sanitization),
105 105 /// stamp `custom_pages_updated_at`, and bump the cache generation. The source is
106 106 /// re-sanitized on render; see [`crate::custom_pages`].
107 + #[tracing::instrument(skip_all, fields(user_id = %id))]
107 108 pub async fn update_user_custom_page<'e>(
108 109 executor: impl sqlx::PgExecutor<'e>,
109 110 id: UserId,