Skip to main content

max / goingson

Re-derive group scope on re-parent, and make sharing validate before it stamps Group scope was derived at INSERT and never again. The three statements that can change a row's parent — task update, bulk_set_project, event update — set project_id and left group_id alone, so: - A task moved out of a shared project kept the old group_id. The changelog trigger reads NEW.group_id, so it went on replicating into the group the user believed they had taken it back from, and stayed readable by every member. - A task moved into a shared project kept group_id NULL and never reached the group at all. Derive it in the same statement, matching every create path (a follow-up UPDATE would double-write the changelog). Child rows carry their own changelog rows, so cascade scope to subtasks, annotations, status tokens, time sessions and attachments when their task moves; bulk_set_project now runs in one transaction so a moved selection cannot settle at a different scope than its children. share_project parsed the group id for shape and reported Ok(true) unconditionally — it never checked the project existed, that the caller owned it, or that the caller was in the group. A stale project id stamped nothing and reported success; a group id the user did not belong to routed the whole subtree into a scope the engine holds no key for, with unshare_project the only way back. Resolve the project first as every other command does, and confirm membership. The nine cascade UPDATEs also carried no user_id predicate, alone among writes in the codebase. Add it, and give the attachments arm explicit parentheses: as written, `A AND B OR C` would have bound as `(A AND B) OR C` and let the task arm skip the check. That arm was reached by prefix-matching the SQL to decide bind counts, which is how it went unstamped once already; the statements now carry their own bind arity.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-01 23:01 UTC
Signed with PGP, not checked
Commit: c3c0566f2b910eb2826e509d83bbdb9a9a7443cd
Parent: 40624cb
3 files changed, +209 insertions, -45 deletions
M src-tauri/src/commands/group.rs +146 -40
@@ -17,8 +17,8 @@
17 17 use tracing::instrument;
18 18 use uuid::Uuid;
19 19
20 - use super::{ApiError, ResultApiError};
21 - use crate::state::AppState;
20 + use super::{ApiError, OptionNotFound, ResultApiError};
21 + use crate::state::{AppState, DESKTOP_USER_ID};
22 22
23 23 /// A group as surfaced to the frontend. `SyncGroup` is deserialize-only and
24 24 /// `#[non_exhaustive]`, so map to this stable DTO for the Tauri boundary.
@@ -174,8 +174,35 @@
174 174 group_id: String,
175 175 ) -> Result<bool, ApiError> {
176 176 // Validate the id shape even though the column is plain TEXT.
177 - parse_uuid("group id", &group_id)?;
178 - set_project_scope(&state.pool, &project_id, Some(&group_id))
177 + let group = parse_uuid("group id", &group_id)?;
178 + let pid = parse_uuid("project id", &project_id)?;
179 +
180 + // Resolve the parent first, the way every other command does. Without this a
181 + // wrong or stale project id stamped zero rows and still reported success, so
182 + // the user believed a project was shared when nothing was.
183 + state
184 + .projects
185 + .get_by_id(pid.into(), DESKTOP_USER_ID)
186 + .await?
187 + .or_not_found("project", pid)?;
188 +
189 + // And confirm the caller is actually in the group. Stamping a scope the sync
190 + // engine holds no key for routes the whole subtree into a changelog scope that
191 + // goes nowhere, and unshare_project is the only way back — which the user has
192 + // no reason to run, having been told the share worked.
193 + let client = require_client(&state)?;
194 + let groups = client
195 + .list_groups()
196 + .await
197 + .map_api_err("Failed to list groups", ApiError::external_service)?;
198 + if !groups.iter().any(|g| g.id == GroupId::from(group)) {
199 + return Err(ApiError::validation(
200 + "groupId",
201 + "You are not a member of that group",
202 + ));
203 + }
204 +
205 + set_project_scope(&state.pool, DESKTOP_USER_ID, &project_id, Some(&group_id))
179 206 .await
180 207 .map_api_err("Failed to share project", ApiError::internal)?;
181 208 Ok(true)
@@ -188,7 +215,13 @@
188 215 state: State<'_, Arc<AppState>>,
189 216 project_id: String,
190 217 ) -> Result<bool, ApiError> {
191 - set_project_scope(&state.pool, &project_id, None)
218 + let pid = parse_uuid("project id", &project_id)?;
219 + state
220 + .projects
221 + .get_by_id(pid.into(), DESKTOP_USER_ID)
222 + .await?
223 + .or_not_found("project", pid)?;
224 + set_project_scope(&state.pool, DESKTOP_USER_ID, &project_id, None)
192 225 .await
193 226 .map_api_err("Failed to unshare project", ApiError::internal)?;
194 227 Ok(true)
@@ -205,33 +238,69 @@
205 238 /// this never changes, so statement order is irrelevant.
206 239 async fn set_project_scope(
207 240 pool: &SqlitePool,
241 + user_id: goingson_core::UserId,
208 242 project_id: &str,
209 243 group_id: Option<&str>,
210 244 ) -> Result<(), sqlx::Error> {
211 - // `WHERE task_id IN (tasks of this project)`.
212 - const BY_TASK: &str = "SELECT id FROM tasks WHERE project_id = ?";
245 + // `WHERE task_id IN (tasks of this project)`. Carries the owner filter too, so
246 + // reaching a row through its task cannot skip the check reaching it directly
247 + // would apply.
248 + const BY_TASK: &str = "SELECT id FROM tasks WHERE project_id = ? AND user_id = ?";
249 +
250 + // Each statement is paired with how many (project_id, user_id) pairs it binds
251 + // after `group_id`, because the arms differ: the direct tables take one, and
252 + // attachments takes two — one for the rows it owns through the project, one for
253 + // the rows it owns through a task. Inferring that from the SQL by prefix match
254 + // is how the attachment arm silently went unstamped.
255 + let statements: [(String, usize); 9] = [
256 + (
257 + "UPDATE projects SET group_id = ? WHERE id = ? AND user_id = ?".to_string(),
258 + 1,
259 + ),
260 + (
261 + "UPDATE tasks SET group_id = ? WHERE project_id = ? AND user_id = ?".to_string(),
262 + 1,
263 + ),
264 + (
265 + "UPDATE milestones SET group_id = ? WHERE project_id = ? AND user_id = ?".to_string(),
266 + 1,
267 + ),
268 + (
269 + "UPDATE events SET group_id = ? WHERE project_id = ? AND user_id = ?".to_string(),
270 + 1,
271 + ),
272 + (
273 + format!("UPDATE subtasks SET group_id = ? WHERE task_id IN ({BY_TASK})"),
274 + 1,
275 + ),
276 + (
277 + format!("UPDATE annotations SET group_id = ? WHERE task_id IN ({BY_TASK})"),
278 + 1,
279 + ),
280 + (
281 + format!("UPDATE task_status_tokens SET group_id = ? WHERE task_id IN ({BY_TASK})"),
282 + 1,
283 + ),
284 + (
285 + format!("UPDATE time_sessions SET group_id = ? WHERE task_id IN ({BY_TASK})"),
286 + 1,
287 + ),
288 + (
289 + // Parenthesised: `A AND B OR C` binds as `(A AND B) OR C`, which would
290 + // let the task arm through without the owner check.
291 + format!(
292 + "UPDATE attachments SET group_id = ? \
293 + WHERE (project_id = ? AND user_id = ?) OR task_id IN ({BY_TASK})"
294 + ),
295 + 2,
296 + ),
297 + ];
213 298
214 299 let mut tx = pool.begin().await?;
215 - for sql in [
216 - "UPDATE projects SET group_id = ? WHERE id = ?".to_string(),
217 - "UPDATE tasks SET group_id = ? WHERE project_id = ?".to_string(),
218 - "UPDATE milestones SET group_id = ? WHERE project_id = ?".to_string(),
219 - "UPDATE events SET group_id = ? WHERE project_id = ?".to_string(),
220 - format!("UPDATE subtasks SET group_id = ? WHERE task_id IN ({BY_TASK})"),
221 - format!("UPDATE annotations SET group_id = ? WHERE task_id IN ({BY_TASK})"),
222 - format!("UPDATE task_status_tokens SET group_id = ? WHERE task_id IN ({BY_TASK})"),
223 - format!("UPDATE time_sessions SET group_id = ? WHERE task_id IN ({BY_TASK})"),
224 - format!(
225 - "UPDATE attachments SET group_id = ? WHERE project_id = ? OR task_id IN ({BY_TASK})"
226 - ),
227 - // attachments binds project_id twice (direct owner + task subquery).
228 - ] {
229 - let attachments = sql.starts_with("UPDATE attachments");
230 - let mut q = sqlx::query(sqlx::AssertSqlSafe(sql.as_str()))
231 - .bind(group_id)
232 - .bind(project_id);
233 - if attachments {
234 - q = q.bind(project_id);
300 + for (sql, pairs) in statements {
301 + let mut q = sqlx::query(sqlx::AssertSqlSafe(sql.as_str())).bind(group_id);
302 + for _ in 0..pairs {
303 + q = q.bind(project_id).bind(user_id.to_string());
235 304 }
236 305 q.execute(&mut *tx).await?;
237 306 }
@@ -243,6 +312,21 @@
243 312 mod tests {
244 313 use super::set_project_scope;
245 314 use sqlx::sqlite::SqlitePoolOptions;
315 + use uuid::Uuid;
316 +
317 + /// The owner every seeded row belongs to.
318 + fn owner() -> goingson_core::UserId {
319 + goingson_core::UserId::from(
320 + Uuid::parse_str("0f0f0f0f-0f0f-4f0f-8f0f-0f0f0f0f0f0f").unwrap(),
321 + )
322 + }
323 +
324 + /// A different user, to prove the cascade's `user_id` predicate actually bites.
325 + fn stranger() -> goingson_core::UserId {
326 + goingson_core::UserId::from(
327 + Uuid::parse_str("0e0e0e0e-0e0e-4e0e-8e0e-0e0e0e0e0e0e").unwrap(),
328 + )
329 + }
246 330
247 331 /// Build an in-memory DB with just the columns the cascade touches, seed a
248 332 /// project subtree plus an unrelated project, and return the pool.
@@ -252,15 +336,15 @@
252 336 .await
253 337 .unwrap();
254 338 sqlx::query(
255 - "CREATE TABLE projects (id TEXT PRIMARY KEY, group_id TEXT);
256 - CREATE TABLE tasks (id TEXT PRIMARY KEY, project_id TEXT, group_id TEXT);
257 - CREATE TABLE milestones (id TEXT PRIMARY KEY, project_id TEXT, group_id TEXT);
258 - CREATE TABLE events (id TEXT PRIMARY KEY, project_id TEXT, group_id TEXT);
339 + "CREATE TABLE projects (id TEXT PRIMARY KEY, user_id TEXT, group_id TEXT);
340 + CREATE TABLE tasks (id TEXT PRIMARY KEY, project_id TEXT, user_id TEXT, group_id TEXT);
341 + CREATE TABLE milestones (id TEXT PRIMARY KEY, project_id TEXT, user_id TEXT, group_id TEXT);
342 + CREATE TABLE events (id TEXT PRIMARY KEY, project_id TEXT, user_id TEXT, group_id TEXT);
259 343 CREATE TABLE subtasks (id TEXT PRIMARY KEY, task_id TEXT, group_id TEXT);
260 344 CREATE TABLE annotations (id TEXT PRIMARY KEY, task_id TEXT, group_id TEXT);
261 345 CREATE TABLE task_status_tokens (id TEXT PRIMARY KEY, task_id TEXT, group_id TEXT);
262 346 CREATE TABLE time_sessions (id TEXT PRIMARY KEY, task_id TEXT, group_id TEXT);
263 - CREATE TABLE attachments (id TEXT PRIMARY KEY, project_id TEXT, task_id TEXT, group_id TEXT);",
347 + CREATE TABLE attachments (id TEXT PRIMARY KEY, project_id TEXT, task_id TEXT, user_id TEXT, group_id TEXT);",
264 348 )
265 349 .execute(&pool)
266 350 .await
@@ -268,16 +352,17 @@
268 352
269 353 // Project P1 with a task T1 and children; an unrelated project P2/T2.
270 354 sqlx::query(
271 - "INSERT INTO projects (id) VALUES ('P1'), ('P2');
272 - INSERT INTO tasks (id, project_id) VALUES ('T1', 'P1'), ('T2', 'P2');
273 - INSERT INTO milestones (id, project_id) VALUES ('M1', 'P1');
274 - INSERT INTO events (id, project_id) VALUES ('E1', 'P1');
355 + "INSERT INTO projects (id, user_id) VALUES ('P1', ?1), ('P2', ?1);
356 + INSERT INTO tasks (id, project_id, user_id) VALUES ('T1', 'P1', ?1), ('T2', 'P2', ?1);
357 + INSERT INTO milestones (id, project_id, user_id) VALUES ('M1', 'P1', ?1);
358 + INSERT INTO events (id, project_id, user_id) VALUES ('E1', 'P1', ?1);
275 359 INSERT INTO subtasks (id, task_id) VALUES ('S1', 'T1'), ('S2', 'T2');
276 360 INSERT INTO annotations (id, task_id) VALUES ('A1', 'T1');
277 361 INSERT INTO task_status_tokens (id, task_id) VALUES ('K1', 'T1');
278 362 INSERT INTO time_sessions (id, task_id) VALUES ('TS1', 'T1');
279 - INSERT INTO attachments (id, project_id, task_id) VALUES ('AT1', 'P1', NULL), ('AT2', NULL, 'T1'), ('AT3', 'P2', NULL);",
363 + INSERT INTO attachments (id, project_id, task_id, user_id) VALUES ('AT1', 'P1', NULL, ?1), ('AT2', NULL, 'T1', ?1), ('AT3', 'P2', NULL, ?1);",
280 364 )
365 + .bind(owner().to_string())
281 366 .execute(&pool)
282 367 .await
283 368 .unwrap();
@@ -315,7 +400,9 @@
315 400 #[tokio::test]
316 401 async fn share_stamps_the_whole_subtree_and_nothing_else() {
317 402 let pool = seed().await;
318 - set_project_scope(&pool, "P1", Some("G")).await.unwrap();
403 + set_project_scope(&pool, owner(), "P1", Some("G"))
404 + .await
405 + .unwrap();
319 406
320 407 for (table, id, gid) in group_ids(&pool).await {
321 408 // Everything under P1 (and P1's attachments AT1/AT2) is stamped; P2's
@@ -330,11 +417,30 @@
330 417 }
331 418 }
332 419
420 + #[tokio::test]
421 + async fn another_users_project_is_untouched() {
422 + let pool = seed().await;
423 + set_project_scope(&pool, stranger(), "P1", Some("G"))
424 + .await
425 + .unwrap();
426 +
427 + // The cascade is the widest write in the codebase; it must key on the owner
428 + // like every other write, not on the project id alone.
429 + for (table, id, gid) in group_ids(&pool).await {
430 + assert_eq!(
431 + gid, None,
432 + "{table}:{id} must stay personal for another user"
433 + );
434 + }
435 + }
436 +
333 437 #[tokio::test]
334 438 async fn unshare_clears_the_whole_subtree() {
335 439 let pool = seed().await;
336 - set_project_scope(&pool, "P1", Some("G")).await.unwrap();
337 - set_project_scope(&pool, "P1", None).await.unwrap();
440 + set_project_scope(&pool, owner(), "P1", Some("G"))
441 + .await
442 + .unwrap();
443 + set_project_scope(&pool, owner(), "P1", None).await.unwrap();
338 444
339 445 for (table, id, gid) in group_ids(&pool).await {
340 446 assert_eq!(gid, None, "{table}:{id} should be personal after unshare");
M crates/db-sqlite/src/repository/event_repo.rs +5 -1
@@ -317,9 +317,13 @@
317 317 };
318 318
319 319 let result = sqlx::query(
320 - "UPDATE events SET project_id = ?, title = ?, description = ?, start_time = ?, end_time = ?, location = ?, linked_task_id = ?, recurrence = ?, recurrence_rule = ?, contact_id = ?, block_type = ?, reminder_offsets_seconds = ?, tz_kind = ?, timezone = ?, start_local = ?, end_local = ? WHERE id = ? AND user_id = ?",
320 + "UPDATE events SET project_id = ?, group_id = (SELECT group_id FROM projects WHERE id = ?), title = ?, description = ?, start_time = ?, end_time = ?, location = ?, linked_task_id = ?, recurrence = ?, recurrence_rule = ?, contact_id = ?, block_type = ?, reminder_offsets_seconds = ?, tz_kind = ?, timezone = ?, start_local = ?, end_local = ? WHERE id = ? AND user_id = ?",
321 321 )
322 322 .bind(event.project_id.map(|p| p.to_string()))
323 + // Re-derive group scope from the new parent, as the create path does.
324 + // Leaving it stale kept a re-parented event replicating into the group it
325 + // was moved out of.
326 + .bind(event.project_id.map(|p| p.to_string()))
323 327 .bind(&event.title)
324 328 .bind(&event.description)
325 329 .bind(&start_str)
M crates/db-sqlite/src/repository/task_repo.rs +58 -4
@@ -327,6 +327,41 @@
327 327 rows_to_tasks(pool, rows).await
328 328 }
329 329
330 + /// Re-derive group scope for the rows hanging off a task, after the task itself
331 + /// changed parent.
332 + ///
333 + /// Scope on these tables is derived from the task at INSERT, so a task that moves
334 + /// between a shared and a personal project leaves its children carrying the old
335 + /// group. They replicate on their own changelog rows, so a stale child keeps
336 + /// leaking after the parent has been corrected.
337 + async fn cascade_task_scope(
338 + tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>,
339 + ids: &[TaskId],
340 + ) -> Result<()> {
341 + if ids.is_empty() {
342 + return Ok(());
343 + }
344 + let placeholders = bind_placeholders(ids.len());
345 + for table in [
346 + "subtasks",
347 + "annotations",
348 + "task_status_tokens",
349 + "time_sessions",
350 + "attachments",
351 + ] {
352 + let sql = format!(
353 + "UPDATE {table} SET group_id = (SELECT group_id FROM tasks WHERE id = {table}.task_id) \
354 + WHERE task_id IN ({placeholders})"
355 + );
356 + let mut q = sqlx::query(sqlx::AssertSqlSafe(sql.as_str()));
357 + for id in ids {
358 + q = q.bind(id.to_string());
359 + }
360 + q.execute(&mut **tx).await.map_err(CoreError::database)?;
361 + }
362 + Ok(())
363 + }
364 +
330 365 #[async_trait]
331 366 impl TaskCrud for SqliteTaskRepository {
332 367 #[tracing::instrument(skip_all)]
@@ -715,11 +750,18 @@
715 750 let result = sqlx::query(
716 751 r"
717 752 UPDATE tasks
718 - SET project_id = ?, contact_id = ?, milestone_id = ?, title = ?, description = ?, status = ?, priority = ?, due = ?, tags = ?, recurrence = ?, recurrence_rule = ?, urgency = ?, scheduled_start = ?, scheduled_duration = ?, estimated_minutes = ?, completed_at = ?
753 + SET project_id = ?, group_id = (SELECT group_id FROM projects WHERE id = ?), contact_id = ?, milestone_id = ?, title = ?, description = ?, status = ?, priority = ?, due = ?, tags = ?, recurrence = ?, recurrence_rule = ?, urgency = ?, scheduled_start = ?, scheduled_duration = ?, estimated_minutes = ?, completed_at = ?
719 754 WHERE id = ? AND user_id = ?
720 755 ",
721 756 )
722 757 .bind(task.project_id.map(|p| p.to_string()))
758 + // Group scope is derived from the parent project, and re-parenting is the one
759 + // path that used to change the parent without re-deriving it: a task moved out
760 + // of a shared project kept the old group_id, so the changelog trigger (which
761 + // reads NEW.group_id) went on replicating it to the group the user believed
762 + // they had taken it back from. Derived in-statement for the same reason every
763 + // create path does it — a follow-up UPDATE would double-write the changelog.
764 + .bind(task.project_id.map(|p| p.to_string()))
723 765 .bind(task.contact_id.map(|c| c.to_string()))
724 766 .bind(task.milestone_id.map(|m| m.to_string()))
725 767 .bind(&task.title)
@@ -742,6 +784,9 @@
742 784 .map_err(CoreError::database)?;
743 785
744 786 let affected = result.rows_affected();
787 + if affected > 0 {
788 + cascade_task_scope(&mut tx, &[id]).await?;
789 + }
745 790 tx.commit().await.map_err(CoreError::database)?;
746 791
747 792 if affected > 0 {
@@ -762,15 +807,24 @@
762 807 return Ok(0);
763 808 }
764 809 let placeholders = bind_placeholders(ids.len());
765 - let sql =
766 - format!("UPDATE tasks SET project_id = ? WHERE user_id = ? AND id IN ({placeholders})");
810 + let sql = format!(
811 + "UPDATE tasks SET project_id = ?, group_id = (SELECT group_id FROM projects WHERE id = ?) \
812 + WHERE user_id = ? AND id IN ({placeholders})"
813 + );
767 814 let mut q = sqlx::query(sqlx::AssertSqlSafe(sql.as_str()))
815 + .bind(project_id.map(|p| p.to_string()))
768 816 .bind(project_id.map(|p| p.to_string()))
769 817 .bind(user_id.to_string());
770 818 for id in ids {
771 819 q = q.bind(id.to_string());
772 820 }
773 - let result = q.execute(&self.pool).await.map_err(CoreError::database)?;
821 + // One transaction: a bulk move drags a whole selection across the sharing
822 + // boundary, so the children must not be able to settle at a different scope
823 + // than their parents.
824 + let mut tx = self.pool.begin().await.map_err(CoreError::database)?;
825 + let result = q.execute(&mut *tx).await.map_err(CoreError::database)?;
826 + cascade_task_scope(&mut tx, ids).await?;
827 + tx.commit().await.map_err(CoreError::database)?;
774 828 Ok(result.rows_affected() as usize)
775 829 }
776 830