diff --git a/backend/tests/drafts_save_follows_move.rs b/backend/tests/drafts_save_follows_move.rs index 259c2f77d9..4e5fe7fdfa 100644 --- a/backend/tests/drafts_save_follows_move.rs +++ b/backend/tests/drafts_save_follows_move.rs @@ -396,29 +396,43 @@ async fn test_a_retried_discard_still_names_the_destination( rename(port, HEAD_HASH, "u/test-user/follow_b").await?; - let discard = || async { - client - .post(format!( - "http://localhost:{port}/api/w/test-workspace/drafts/update/script/u/test-user/follow_a" - )) - .header("Authorization", "Bearer SECRET_TOKEN") - .json(&json!({ "value": null })) - .send() - .await? - .json::() - .await + let discard = |token: &'static str| { + let client = client.clone(); + async move { + client + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/update/script/u/test-user/follow_a" + )) + .header("Authorization", format!("Bearer {token}")) + .json(&json!({ "value": null })) + .send() + .await? + .json::() + .await + } }; - let first = discard().await?; + let first = discard("SECRET_TOKEN").await?; assert_eq!(first["status"], "saved", "{first}"); assert_eq!(first["path"], "u/test-user/follow_b", "{first}"); - let retry = discard().await?; + let retry = discard("SECRET_TOKEN").await?; assert_eq!( retry["status"], "saved", "the retry was not idempotent: {retry}" ); assert_eq!(retry["path"], "u/test-user/follow_b", "{retry}"); + + // A third user has no draft on this item, so their discard deletes nothing and the + // destination is a move record and nothing else: it is answered only to a caller who + // could write there. + let other = discard("SECRET_TOKEN_3").await?; + assert_eq!(other["status"], "saved", "{other}"); + assert_eq!( + other["path"], + Value::Null, + "a user who cannot write the destination was told where the item went: {other}" + ); Ok(()) } diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index a0ed5888e3..d26f372023 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -579,13 +579,27 @@ async fn update_draft( let now = sqlx::query_scalar!(r#"SELECT now() as "now!""#) .fetch_one(&db) .await?; + // A retry of a routed discard whose answer was lost lands here: the row is + // gone but the editor is still on the path the item left, so it needs the + // destination as much as the first attempt did. Unlike the arm above there is + // no deleted row proving the caller ever held that draft, and an own discard + // skips the write check, so name the destination only to someone who could + // write there. Otherwise any member could read `draft_move` by discarding at + // a guessed path. + let disclosed = match moved_to { + Some(dest) => { + match require_can_write_path(&authed, &db, &user_db, &w_id, kind, &dest).await { + Ok(()) => Some(dest), + Err(Error::NotAuthorized(_)) | Err(Error::BadRequest(_)) => None, + Err(e) => return Err(e), + } + } + None => None, + }; Ok(Json(SaveDraftResponse { status: SaveDraftStatus::Saved, current_timestamp: now, - // A retry of a routed discard whose answer was lost lands here: the row - // is gone but the editor is still on the path the item left, so it needs - // the destination as much as the first attempt did. - path: moved_to, + path: disclosed, })) } }