From b3ce81945621e9026e687b590e7aa541008575ac Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 1 Jul 2025 00:31:09 -0400 Subject: Sending messages to a deleted channel should send to the deleted channel's ID, not to a fictitious ID. The existing test scenario: * Create a channel (with ID C1). * Delete channel C1. * Roll the dice to invent a channel ID C2. * Send a message to channel C2. * Observe that sending fails. This was not verifying anything about the deleted channel C1 - it was basically reproducing the `nonexistent_channel` test scenario with the most marginal of garnishes on it. This is probably copy-paste damage from when this test was originally written. Sending did fail, so this test scenario passed, but it passed effectively by accident. The new test scenario: * Create a channel (with ID C1). * Delete channel C1. * Send a message to channel C1. * Observe that sending fails. Concerningly, sending does _not_ fail in this scenario (i.e., the test _does_ fail), so the broken test case was masking a real bug. --- src/channel/handlers/send/test.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/channel/handlers/send/test.rs b/src/channel/handlers/send/test.rs index 7204ca4..d77e07d 100644 --- a/src/channel/handlers/send/test.rs +++ b/src/channel/handlers/send/test.rs @@ -108,13 +108,12 @@ async fn deleted_channel() { // Call the endpoint let sent_at = fixtures::now(); - let channel = channel::Id::generate(); let request = super::Request { body: fixtures::message::propose(), }; let super::Error(error) = super::handler( State(app), - Path(channel.clone()), + Path(channel.id.clone()), sent_at, sender, Json(request), @@ -126,6 +125,6 @@ async fn deleted_channel() { assert!(matches!( error, - SendError::ChannelNotFound(error_channel) if channel == error_channel + SendError::ChannelNotFound(error_channel) if channel.id == error_channel )); } -- cgit v1.2.3 From b4db819ef8daa583a165aed01eb3d70d98e37fc8 Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 1 Jul 2025 01:42:38 -0400 Subject: Prevent sending messages to deleted channels. I've opted to make it clear in the error message which scenario - deleted vs. non-existant - a channel falls into. This isn't particularly consistent with the rest of the API, so we might need to review this decision later, but it's at least relatively harmless if it's mistaken. (Formally, they're both 404s, so clients that go by error code won't notice.) --- src/channel/app.rs | 8 ++++---- src/channel/handlers/send/mod.rs | 4 +++- src/channel/handlers/send/test.rs | 2 +- src/channel/history.rs | 9 +++++++++ src/message/app.rs | 16 +++++++++++----- src/message/repo.rs | 7 +++---- 6 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/channel/app.rs b/src/channel/app.rs index dc9e584..e3b169c 100644 --- a/src/channel/app.rs +++ b/src/channel/app.rs @@ -48,14 +48,14 @@ impl<'a> Channels<'a> { // This function is careless with respect to time, and gets you the channel as // it exists in the specific moment when you call it. pub async fn get(&self, channel: &Id) -> Result { - let not_found = || Error::NotFound(channel.clone()); - let deleted = || Error::Deleted(channel.clone()); + let to_not_found = || Error::NotFound(channel.clone()); + let to_deleted = || Error::Deleted(channel.clone()); let mut tx = self.db.begin().await?; - let channel = tx.channels().by_id(channel).await.not_found(not_found)?; + let channel = tx.channels().by_id(channel).await.not_found(to_not_found)?; tx.commit().await?; - channel.as_snapshot().ok_or_else(deleted) + channel.as_snapshot().ok_or_else(to_deleted) } pub async fn delete(&self, channel: &Id, deleted_at: &DateTime) -> Result<(), DeleteError> { diff --git a/src/channel/handlers/send/mod.rs b/src/channel/handlers/send/mod.rs index aa241e2..bde39e5 100644 --- a/src/channel/handlers/send/mod.rs +++ b/src/channel/handlers/send/mod.rs @@ -54,7 +54,9 @@ impl IntoResponse for Error { fn into_response(self) -> response::Response { let Self(error) = self; match error { - SendError::ChannelNotFound(_) => NotFound(error).into_response(), + SendError::ChannelNotFound(_) | SendError::ChannelDeleted(_) => { + NotFound(error).into_response() + } SendError::Name(_) | SendError::Database(_) => Internal::from(error).into_response(), } } diff --git a/src/channel/handlers/send/test.rs b/src/channel/handlers/send/test.rs index d77e07d..70d45eb 100644 --- a/src/channel/handlers/send/test.rs +++ b/src/channel/handlers/send/test.rs @@ -125,6 +125,6 @@ async fn deleted_channel() { assert!(matches!( error, - SendError::ChannelNotFound(error_channel) if channel.id == error_channel + SendError::ChannelDeleted(error_channel) if channel.id == error_channel )); } diff --git a/src/channel/history.rs b/src/channel/history.rs index 7f18e45..85da5a5 100644 --- a/src/channel/history.rs +++ b/src/channel/history.rs @@ -27,6 +27,15 @@ impl History { self.channel.clone() } + pub fn as_of(&self, sequence: S) -> Option + where + S: Into, + { + self.events() + .filter(Sequence::up_to(sequence.into())) + .collect() + } + // Snapshot of this channel as of all events recorded in this history. pub fn as_snapshot(&self) -> Option { self.events().collect() diff --git a/src/message/app.rs b/src/message/app.rs index 3c74628..9792c8f 100644 --- a/src/message/app.rs +++ b/src/message/app.rs @@ -29,13 +29,17 @@ impl<'a> Messages<'a> { sent_at: &DateTime, body: &Body, ) -> Result { + let to_not_found = || SendError::ChannelNotFound(channel.clone()); + let to_deleted = || SendError::ChannelDeleted(channel.clone()); + let mut tx = self.db.begin().await?; - let channel = tx - .channels() - .by_id(channel) - .await - .not_found(|| SendError::ChannelNotFound(channel.clone()))?; + let channel = tx.channels().by_id(channel).await.not_found(to_not_found)?; + + // Ordering: don't bother allocating a sequence number before we know the channel might + // exist. let sent = tx.sequence().next(sent_at).await?; + let channel = channel.as_of(sent).ok_or_else(to_deleted)?; + let message = tx.messages().create(&channel, sender, &sent, body).await?; tx.commit().await?; @@ -126,6 +130,8 @@ impl<'a> Messages<'a> { pub enum SendError { #[error("channel {0} not found")] ChannelNotFound(channel::Id), + #[error("channel {0} deleted")] + ChannelDeleted(channel::Id), #[error(transparent)] Database(#[from] sqlx::Error), #[error(transparent)] diff --git a/src/message/repo.rs b/src/message/repo.rs index 9a4f72f..e753134 100644 --- a/src/message/repo.rs +++ b/src/message/repo.rs @@ -2,7 +2,7 @@ use sqlx::{SqliteConnection, Transaction, sqlite::Sqlite}; use super::{Body, History, Id, snapshot::Message}; use crate::{ - channel, + channel::{self, Channel}, clock::DateTime, event::{Instant, Sequence}, user::{self, User}, @@ -23,13 +23,12 @@ pub struct Messages<'t>(&'t mut SqliteConnection); impl Messages<'_> { pub async fn create( &mut self, - channel: &channel::History, + channel: &Channel, sender: &User, sent: &Instant, body: &Body, ) -> Result { let id = Id::generate(); - let channel_id = channel.id(); let message = sqlx::query!( r#" @@ -45,7 +44,7 @@ impl Messages<'_> { body as "body: Body" "#, id, - channel_id, + channel.id, sender.id, sent.at, sent.sequence, -- cgit v1.2.3