max / makenotwork
7 files changed,
+98 insertions,
-42 deletions
| @@ -1,15 +1,24 @@ | |||
| 1 | - | # Seal the C1 resource-in-community invariant (ultra-fuzz CHRONIC). | |
| 1 | + | # The C1 resource-in-community invariant (ultra-fuzz CHRONIC) is sealed by the | |
| 2 | + | # type system now, with one lint-guarded escape hatch. | |
| 2 | 3 | # | |
| 3 | - | # These raw by-id loaders return a community resource without proving it belongs | |
| 4 | - | # to the URL slug's community. Handler code must instead go through | |
| 5 | - | # `crate::routes::scope::CommunityScope::resolve`, which performs the equality | |
| 6 | - | # check. The only sanctioned callers are the `ScopedResource::load` impls in | |
| 7 | - | # `src/routes/scope.rs`, which carry a local `#[allow(clippy::disallowed_methods)]`. | |
| 4 | + | # The raw by-id loaders (`get_thread_with_breadcrumb`, `get_post_for_edit`) | |
| 5 | + | # return their resource wrapped in `mt_db::queries::Unscoped<T>`. The inner value | |
| 6 | + | # is private; the safe way out is `Unscoped::in_community(expected)`, which yields | |
| 7 | + | # it only when the resource's community matches the one the caller names. | |
| 8 | + | # `routes::scope::CommunityScope::resolve` is the sole such site and it passes the | |
| 9 | + | # community the URL slug resolved to, so a handler cannot serve a by-id community | |
| 10 | + | # resource without proving it belongs to that community. That check needs no lint: | |
| 11 | + | # it is enforced by the type. | |
| 8 | 12 | # | |
| 9 | - | # Under the `-D warnings` CI gate, calling either from anywhere else is a build | |
| 10 | - | # failure — so a handler that loads a community resource without the check no | |
| 11 | - | # longer compiles. That is the structural resolution of C1. | |
| 13 | + | # `Unscoped::into_inner_unchecked` unwraps with no community check, for callers | |
| 14 | + | # that have no slug to scope against (the trusted internal server-to-server API). | |
| 15 | + | # It is disallowed below so a `/p/{slug}/…` handler cannot reach for it by | |
| 16 | + | # accident; the few sanctioned sites carry a local `#[allow]`. | |
| 17 | + | # | |
| 18 | + | # This replaced a deny-list that enumerated every raw loader by hand and failed | |
| 19 | + | # open for any loader nobody remembered to add. The seal now fails closed: a new | |
| 20 | + | # by-id loader that returns `Unscoped<T>` is bound automatically, and the only | |
| 21 | + | # lint entry is on the single unchecked-unwrap primitive. See `src/routes/scope.rs`. | |
| 12 | 22 | disallowed-methods = [ | |
| 13 | - | { path = "mt_db::queries::get_thread_with_breadcrumb", reason = "load via routes::scope::CommunityScope so the resource is proven to belong to the URL slug's community (C1)" }, | |
| 14 | - | { path = "mt_db::queries::get_post_for_edit", reason = "load via routes::scope::CommunityScope so the resource is proven to belong to the URL slug's community (C1)" }, | |
| 23 | + | { path = "mt_db::queries::Unscoped::into_inner_unchecked", reason = "unwraps a community resource without the C1 scope check; a slug-scoped handler must use Unscoped::in_community via routes::scope::CommunityScope instead" }, | |
| 15 | 24 | ] |
| @@ -5,6 +5,48 @@ use mt_core::types::{BanType, CommunityRole, CommunityState, ModAction, SortColu | |||
| 5 | 5 | use sqlx::PgPool; | |
| 6 | 6 | use uuid::Uuid; | |
| 7 | 7 | ||
| 8 | + | /// A community resource loaded by id but not yet proven to belong to a caller's | |
| 9 | + | /// community scope (the ultra-fuzz C1 invariant). | |
| 10 | + | /// | |
| 11 | + | /// A `/p/{slug}/…/{resource_id}` handler must serve a resource whose community | |
| 12 | + | /// equals the one the `{slug}` names. `Unscoped<T>` makes that the *only* way to | |
| 13 | + | /// obtain the resource: the inner value is private and [`Unscoped::in_community`] | |
| 14 | + | /// yields it only when the resource's community matches the one the caller names. | |
| 15 | + | /// A handler therefore cannot read a by-id resource without proving it belongs to | |
| 16 | + | /// the URL's community, and the seal fails closed by type: any new by-id loader | |
| 17 | + | /// that returns `Unscoped<T>` is bound by the same rule with no lint to remember. | |
| 18 | + | pub struct Unscoped<T> { | |
| 19 | + | inner: T, | |
| 20 | + | community_id: Uuid, | |
| 21 | + | } | |
| 22 | + | ||
| 23 | + | impl<T> Unscoped<T> { | |
| 24 | + | /// Wrap a freshly-loaded resource together with the community it lives in. | |
| 25 | + | pub fn new(inner: T, community_id: Uuid) -> Self { | |
| 26 | + | Self { | |
| 27 | + | inner, | |
| 28 | + | community_id, | |
| 29 | + | } | |
| 30 | + | } | |
| 31 | + | ||
| 32 | + | /// Yield the resource only if it belongs to `expected_community`; `None` | |
| 33 | + | /// otherwise (the id names a resource in a different community, which to a | |
| 34 | + | /// scoped URL is indistinguishable from "not found"). | |
| 35 | + | pub fn in_community(self, expected_community: Uuid) -> Option<T> { | |
| 36 | + | (self.community_id == expected_community).then_some(self.inner) | |
| 37 | + | } | |
| 38 | + | ||
| 39 | + | /// Unwrap without a community check. This is the deliberate escape hatch for | |
| 40 | + | /// callers that have no URL slug to scope against, the trusted internal | |
| 41 | + | /// server-to-server API. It is on `clippy.toml`'s `disallowed-methods` so a | |
| 42 | + | /// normal handler cannot reach for it by accident; the few sanctioned call | |
| 43 | + | /// sites carry a local `#[allow]`. Anything reached via `/p/{slug}/…` must use | |
| 44 | + | /// [`Unscoped::in_community`] instead. | |
| 45 | + | pub fn into_inner_unchecked(self) -> T { | |
| 46 | + | self.inner | |
| 47 | + | } | |
| 48 | + | } | |
| 49 | + | ||
| 8 | 50 | mod admin; | |
| 9 | 51 | mod category; | |
| 10 | 52 | mod community; |
| @@ -132,7 +132,7 @@ pub async fn count_recent_posts_by_user( | |||
| 132 | 132 | pub async fn get_post_for_edit( | |
| 133 | 133 | pool: &PgPool, | |
| 134 | 134 | post_id: Uuid, | |
| 135 | - | ) -> Result<Option<PostForEdit>, sqlx::Error> { | |
| 135 | + | ) -> Result<Option<super::Unscoped<PostForEdit>>, sqlx::Error> { | |
| 136 | 136 | sqlx::query_as!( | |
| 137 | 137 | PostForEdit, | |
| 138 | 138 | r#"SELECT p.id, p.author_id, p.body_markdown, | |
| @@ -151,6 +151,12 @@ pub async fn get_post_for_edit( | |||
| 151 | 151 | ) | |
| 152 | 152 | .fetch_optional(pool) | |
| 153 | 153 | .await | |
| 154 | + | .map(|opt| { | |
| 155 | + | opt.map(|row| { | |
| 156 | + | let community_id = row.community_id; | |
| 157 | + | super::Unscoped::new(row, community_id) | |
| 158 | + | }) | |
| 159 | + | }) | |
| 154 | 160 | } | |
| 155 | 161 | ||
| 156 | 162 | /// Fetch a post's author_id and body_markdown for quote verification, scoped to |
| @@ -191,7 +191,7 @@ pub async fn count_threads_in_category( | |||
| 191 | 191 | pub async fn get_thread_with_breadcrumb( | |
| 192 | 192 | pool: &PgPool, | |
| 193 | 193 | thread_id: Uuid, | |
| 194 | - | ) -> Result<Option<ThreadWithBreadcrumb>, sqlx::Error> { | |
| 194 | + | ) -> Result<Option<super::Unscoped<ThreadWithBreadcrumb>>, sqlx::Error> { | |
| 195 | 195 | sqlx::query_as!( | |
| 196 | 196 | ThreadWithBreadcrumb, | |
| 197 | 197 | r#"SELECT t.id, t.title, t.locked, t.pinned, t.author_id, | |
| @@ -206,6 +206,12 @@ pub async fn get_thread_with_breadcrumb( | |||
| 206 | 206 | ) | |
| 207 | 207 | .fetch_optional(pool) | |
| 208 | 208 | .await | |
| 209 | + | .map(|opt| { | |
| 210 | + | opt.map(|row| { | |
| 211 | + | let community_id = row.community_id; | |
| 212 | + | super::Unscoped::new(row, community_id) | |
| 213 | + | }) | |
| 214 | + | }) | |
| 209 | 215 | } | |
| 210 | 216 | ||
| 211 | 217 | /// Count threads in a category, optionally filtered by tag slug. |
| @@ -328,11 +328,12 @@ async fn create_post( | |||
| 328 | 328 | // server-to-server API (no URL slug to scope against), so the CommunityScope | |
| 329 | 329 | // resolver doesn't apply, the thread id is supplied directly by the trusted | |
| 330 | 330 | // internal caller, not derived from a `/p/{slug}/…` path. | |
| 331 | - | #[allow(clippy::disallowed_methods)] // internal API: no slug to verify against | |
| 332 | 331 | let thread_info = mt_db::queries::get_thread_with_breadcrumb(&state.db, thread_id) | |
| 333 | 332 | .await | |
| 334 | 333 | .map_err(db_error)? | |
| 335 | 334 | .ok_or_else(|| (StatusCode::NOT_FOUND, "Thread not found").into_response())?; | |
| 335 | + | #[allow(clippy::disallowed_methods)] // internal API: no slug to verify against | |
| 336 | + | let thread_info = thread_info.into_inner_unchecked(); | |
| 336 | 337 | ||
| 337 | 338 | mt_db::mutations::ensure_membership(&state.db, req.author_mnw_id, thread_info.community_id) | |
| 338 | 339 | .await |
| @@ -24,53 +24,44 @@ | |||
| 24 | 24 | //! ([`CommunityScope::require_write_access`], [`CommunityScope::role`]) read the | |
| 25 | 25 | //! community *from that value*, so they cannot be handed a divergent community | |
| 26 | 26 | //! id. The raw by-id loaders (`get_thread_with_breadcrumb`, `get_post_for_edit`) | |
| 27 | - | //! are barred from handler code by a `disallowed-methods` clippy lint (see | |
| 28 | - | //! `clippy.toml`) that is allowed *only* inside this module, so a handler that | |
| 29 | - | //! loads a community resource without the equality check no longer compiles | |
| 30 | - | //! under the `-D warnings` gate. | |
| 27 | + | //! return their resource wrapped in [`mt_db::queries::Unscoped`]: the inner value | |
| 28 | + | //! is private and only [`Unscoped::in_community`] yields it, and only when the | |
| 29 | + | //! resource's community matches the one the caller names. A handler therefore | |
| 30 | + | //! cannot read a by-id community resource without proving it belongs to the URL's | |
| 31 | + | //! community — the C1 equality check *is* the unwrap. The seal is by type, not a | |
| 32 | + | //! lint, so it fails closed: a new by-id loader returning `Unscoped<T>` is bound | |
| 33 | + | //! by the same rule automatically. | |
| 31 | 34 | ||
| 32 | 35 | use axum::response::{IntoResponse, Response}; | |
| 33 | 36 | use uuid::Uuid; | |
| 34 | 37 | ||
| 35 | 38 | use mt_core::types::CommunityRole; | |
| 36 | - | use mt_db::queries::{CommunityRow, PostForEdit, ThreadWithBreadcrumb}; | |
| 39 | + | use mt_db::queries::{CommunityRow, PostForEdit, ThreadWithBreadcrumb, Unscoped}; | |
| 37 | 40 | ||
| 38 | 41 | use super::{check_write_access, db_error, get_community, get_role, is_mod_or_owner, parse_uuid}; | |
| 39 | 42 | ||
| 40 | 43 | /// A resource that lives inside a community and can be loaded by id. | |
| 41 | 44 | /// | |
| 42 | 45 | /// Implemented only for the resource types that hang off a `/p/{slug}/…` path. | |
| 43 | - | /// The `load` impls are the sole sanctioned callers of the raw mt-db by-id | |
| 44 | - | /// loaders; everything else must go through [`CommunityScope`]. | |
| 46 | + | /// `load` returns the resource still wrapped in [`Unscoped`]; only | |
| 47 | + | /// [`CommunityScope::resolve`] unwraps it, against the slug's community. | |
| 45 | 48 | pub(crate) trait ScopedResource: Sized + Send { | |
| 46 | 49 | fn load( | |
| 47 | 50 | db: &sqlx::PgPool, | |
| 48 | 51 | id: Uuid, | |
| 49 | - | ) -> impl std::future::Future<Output = Result<Option<Self>, sqlx::Error>> + Send; | |
| 50 | - | ||
| 51 | - | fn community_id(&self) -> Uuid; | |
| 52 | + | ) -> impl std::future::Future<Output = Result<Option<Unscoped<Self>>, sqlx::Error>> + Send; | |
| 52 | 53 | } | |
| 53 | 54 | ||
| 54 | 55 | impl ScopedResource for ThreadWithBreadcrumb { | |
| 55 | - | async fn load(db: &sqlx::PgPool, id: Uuid) -> Result<Option<Self>, sqlx::Error> { | |
| 56 | - | #[allow(clippy::disallowed_methods)] // the one sanctioned call site | |
| 56 | + | async fn load(db: &sqlx::PgPool, id: Uuid) -> Result<Option<Unscoped<Self>>, sqlx::Error> { | |
| 57 | 57 | mt_db::queries::get_thread_with_breadcrumb(db, id).await | |
| 58 | 58 | } | |
| 59 | - | ||
| 60 | - | fn community_id(&self) -> Uuid { | |
| 61 | - | self.community_id | |
| 62 | - | } | |
| 63 | 59 | } | |
| 64 | 60 | ||
| 65 | 61 | impl ScopedResource for PostForEdit { | |
| 66 | - | async fn load(db: &sqlx::PgPool, id: Uuid) -> Result<Option<Self>, sqlx::Error> { | |
| 67 | - | #[allow(clippy::disallowed_methods)] // the one sanctioned call site | |
| 62 | + | async fn load(db: &sqlx::PgPool, id: Uuid) -> Result<Option<Unscoped<Self>>, sqlx::Error> { | |
| 68 | 63 | mt_db::queries::get_post_for_edit(db, id).await | |
| 69 | 64 | } | |
| 70 | - | ||
| 71 | - | fn community_id(&self) -> Uuid { | |
| 72 | - | self.community_id | |
| 73 | - | } | |
| 74 | 65 | } | |
| 75 | 66 | ||
| 76 | 67 | /// A resource verified to belong to the community its URL slug names. | |
| @@ -94,15 +85,15 @@ impl<T: ScopedResource> CommunityScope<T> { | |||
| 94 | 85 | ) -> Result<Self, Response> { | |
| 95 | 86 | let community = get_community(db, slug).await?; | |
| 96 | 87 | let id = parse_uuid(resource_id_str)?; | |
| 88 | + | // `in_community` is the C1 check: it yields the resource only if it lives | |
| 89 | + | // in the slug's community. A resource that exists in a *different* | |
| 90 | + | // community unwraps to `None`, indistinguishable from "not found" to this | |
| 91 | + | // URL, exactly as a missing id is. | |
| 97 | 92 | let resource = T::load(db, id) | |
| 98 | 93 | .await | |
| 99 | 94 | .map_err(db_error)? | |
| 95 | + | .and_then(|scoped| scoped.in_community(community.id)) | |
| 100 | 96 | .ok_or_else(crate::error_page::not_found)?; | |
| 101 | - | if resource.community_id() != community.id { | |
| 102 | - | // The resource exists but lives in a different community than the | |
| 103 | - | // slug names, indistinguishable from "not found" to this URL. | |
| 104 | - | return Err(crate::error_page::not_found()); | |
| 105 | - | } | |
| 106 | 97 | Ok(Self { | |
| 107 | 98 | community, | |
| 108 | 99 | resource, |
| @@ -192,7 +192,8 @@ async fn mod_can_remove_post() { | |||
| 192 | 192 | let post_data = mt_db::queries::get_post_for_edit(&h.db, post_id) | |
| 193 | 193 | .await | |
| 194 | 194 | .unwrap() | |
| 195 | - | .unwrap(); | |
| 195 | + | .unwrap() | |
| 196 | + | .into_inner_unchecked(); | |
| 196 | 197 | assert_eq!( | |
| 197 | 198 | post_data.body_markdown, "Problematic content", | |
| 198 | 199 | "Content should be preserved" |