Stop CDN-caching the private feed and couple cache invalidation to deletion (ultra-fuzz Run 12 Security)
Three security-axis fixes from Run 12 + its doubledown:
- /feed cross-user leak: bare /feed is the AuthUser, per-viewer follows page but
matched is_public_page, so the cache middleware stamped it public/s-maxage=60 —
a shared CDN could serve one user's feed to another. Narrow the match to
/feed/ so the per-URL HMAC-signed RSS feed stays cacheable while the private
page falls back to private/no-cache.
- domain_cache staleness (S-E): account deletion cascaded the custom_domains
rows but never purged the in-memory domain_cache, which has no TTL and never
re-validates a hit. Add AppState::delete_user_account as the single deletion
entry point (DB delete + cache purge) and seal db::users::delete_user to
pub(crate) so a caller cannot delete a user and leave the cache stale.
- Fix a stale SyncAppKeyExtractor doc comment that described a removed
unverified-fallback path; the code collapses to the nil sentinel bucket.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7 files changed,
+33 insertions,
-8 deletions
| 245 |
245 |
|
}
|
| 246 |
246 |
|
|
| 247 |
247 |
|
/// Permanently delete a user by ID.
|
|
248 |
+ |
///
|
|
249 |
+ |
/// `pub(crate)` and not for direct handler use: go through
|
|
250 |
+ |
/// [`crate::AppState::delete_user_account`], which also purges the in-memory
|
|
251 |
+ |
/// caches keyed to the user (domain_cache). Deleting here alone would leave a
|
|
252 |
+ |
/// stale, never-revalidated cache entry (ultra-fuzz Run 12 doubledown S-E).
|
| 248 |
253 |
|
#[tracing::instrument(skip_all)]
|
| 249 |
|
- |
pub async fn delete_user(pool: &PgPool, id: UserId) -> Result<()> {
|
|
254 |
+ |
pub(crate) async fn delete_user(pool: &PgPool, id: UserId) -> Result<()> {
|
| 250 |
255 |
|
sqlx::query("DELETE FROM users WHERE id = $1")
|
| 251 |
256 |
|
.bind(id)
|
| 252 |
257 |
|
.execute(pool)
|
| 152 |
152 |
|
.as_ref()
|
| 153 |
153 |
|
.ok_or_else(|| error::AppError::ServiceUnavailable("SyncKit storage is not configured".to_string()))
|
| 154 |
154 |
|
}
|
|
155 |
+ |
|
|
156 |
+ |
/// Delete a user account and purge every derived in-memory cache keyed to it.
|
|
157 |
+ |
///
|
|
158 |
+ |
/// This is the single deletion entry point for handlers and the scheduler. The
|
|
159 |
+ |
/// raw `db::users::delete_user` is `pub(crate)` so a caller cannot delete a user
|
|
160 |
+ |
/// and forget to purge `domain_cache` — which has no TTL and never re-validates a
|
|
161 |
+ |
/// hit, so a stale entry would otherwise linger until process restart (ultra-fuzz
|
|
162 |
+ |
/// Run 12 doubledown S-E: couple cache invalidation to the mutation). The custom
|
|
163 |
+ |
/// domain rows cascade at the DB layer; here we drop the matching cache entries.
|
|
164 |
+ |
pub async fn delete_user_account(&self, user_id: db::UserId) -> error::Result<()> {
|
|
165 |
+ |
db::users::delete_user(&self.db, user_id).await?;
|
|
166 |
+ |
self.domain_cache.retain(|_, uid| *uid != user_id);
|
|
167 |
+ |
Ok(())
|
|
168 |
+ |
}
|
| 155 |
169 |
|
}
|
| 156 |
170 |
|
|
| 157 |
171 |
|
/// Build the app router with all routes and middleware (minus tracing/TCP).
|
| 75 |
75 |
|
|| path.starts_with("/docs")
|
| 76 |
76 |
|
|| path.starts_with("/discover/")
|
| 77 |
77 |
|
|| path.starts_with("/source/")
|
| 78 |
|
- |
|| path.starts_with("/feed")
|
|
78 |
+ |
// `/feed/{user_id}` is a per-URL, HMAC-signed, viewer-independent RSS feed —
|
|
79 |
+ |
// safe to CDN-cache. Bare `/feed` is the AuthUser, per-viewer follows page:
|
|
80 |
+ |
// it must NOT be stamped `public`, or a shared CDN could serve one user's
|
|
81 |
+ |
// private feed to another (ultra-fuzz Run 12 doubledown cross-cutting).
|
|
82 |
+ |
|| path.starts_with("/feed/")
|
| 79 |
83 |
|
}
|
| 80 |
84 |
|
|
| 81 |
85 |
|
/// Axum middleware that records HTTP request metrics.
|
| 63 |
63 |
|
#[derive(Debug, Clone)]
|
| 64 |
64 |
|
pub struct SyncAppKeyExtractor {
|
| 65 |
65 |
|
/// SyncKit JWT signing secret. `None` when SyncKit isn't configured — in
|
| 66 |
|
- |
/// that case there is no auth and these routes are inert, so we fall back
|
| 67 |
|
- |
/// to the unverified payload parse to preserve prior behavior.
|
|
66 |
+ |
/// that case there is no secret to verify against, so every token collapses
|
|
67 |
+ |
/// into the shared nil sentinel bucket (the anti-spray behavior), pinned by
|
|
68 |
+ |
/// the `no_secret_collapses_unverified_app_to_nil` test. (An earlier version
|
|
69 |
+ |
/// fell back to an unverified payload parse; that was removed.)
|
| 68 |
70 |
|
secret: Option<std::sync::Arc<String>>,
|
| 69 |
71 |
|
}
|
| 70 |
72 |
|
|
| 266 |
266 |
|
crate::email::send_creator_departure_notifications(&pool, &email, user_id, creator_name).await;
|
| 267 |
267 |
|
});
|
| 268 |
268 |
|
} else {
|
| 269 |
|
- |
db::users::delete_user(&state.db, user.id).await?;
|
|
269 |
+ |
state.delete_user_account(user.id).await?;
|
| 270 |
270 |
|
}
|
| 271 |
271 |
|
|
| 272 |
272 |
|
Ok(StatusCode::NO_CONTENT)
|
| 176 |
176 |
|
crate::email::send_creator_departure_notifications(&pool, &email_client, user_id, creator_name).await;
|
| 177 |
177 |
|
});
|
| 178 |
178 |
|
} else {
|
| 179 |
|
- |
db::users::delete_user(&state.db, user_id).await?;
|
|
179 |
+ |
state.delete_user_account(user_id).await?;
|
| 180 |
180 |
|
tracing::info!(user_id = %user_id, event = "account_deleted", "Account permanently deleted via confirmed POST");
|
| 181 |
181 |
|
}
|
| 182 |
182 |
|
|
| 148 |
148 |
|
cleanup_git_repos_on_disk(git_root, &user.username, user_id).await;
|
| 149 |
149 |
|
}
|
| 150 |
150 |
|
|
| 151 |
|
- |
// CASCADE delete user row
|
| 152 |
|
- |
if let Err(e) = db::users::delete_user(&state.db, user_id).await {
|
|
151 |
+ |
// CASCADE delete user row (+ purge domain_cache via the coupled entry point)
|
|
152 |
+ |
if let Err(e) = state.delete_user_account(user_id).await {
|
| 153 |
153 |
|
tracing::error!(error = ?e, %user_id, "{label}: failed to delete account");
|
| 154 |
154 |
|
false
|
| 155 |
155 |
|
} else {
|