From e3dc8ee327946a247a00f03e28ac999ac744ca6f Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 17 Aug 2026 08:04:33 +0900 Subject: [PATCH] fix: validate merge authority before ref updates --- crates/merge-request/src/lib.rs | 69 +++++++++++++++++++++++++++ crates/merge-request/tests/store.rs | 48 +++++++++++++++++++ crates/workspace-server/src/server.rs | 30 +++++++----- 3 files changed, 134 insertions(+), 13 deletions(-) diff --git a/crates/merge-request/src/lib.rs b/crates/merge-request/src/lib.rs index ddb67bc4..c86af168 100644 --- a/crates/merge-request/src/lib.rs +++ b/crates/merge-request/src/lib.rs @@ -256,6 +256,7 @@ pub struct RepairSelectorFrom { pub workspace_id: String, pub ticket_id: String, pub selector_from: String, + pub resolved_subject_ref: String, pub repaired_by: WorkerIdentity, pub reason: String, pub now: DateTime, @@ -638,7 +639,66 @@ impl MergeRequestStore { review, }) } + pub fn validate_completion(&self, i: &CompleteMergeRequest) -> Result<(), MergeRequestError> { + let mr = self.get(&i.auth.workspace_id, &i.ticket_id)?; + self.completion_auth(&i.auth, &i.ticket_id, &mr.repository_id)?; + if let Some(existing) = mr.thread.iter().find_map(|event| match event { + MergeRequestThreadEvent::Merge(value) if value.operation_id == i.operation_id => { + Some(value) + } + _ => None, + }) { + if existing.approval_event_id == i.approval_event_id + && existing.target_ref_before == i.target_ref_before + && existing.target_ref_after == i.target_ref_after + { + return Ok(()); + } + return Err(MergeRequestError::Conflict( + "operation fingerprint mismatch".into(), + )); + } + if mr.state != MergeRequestState::Open { + return Err(MergeRequestError::Conflict( + "Merge Request is not open".into(), + )); + } + let review = mr + .effective_review(&i.current_subject_ref) + .filter(|review| review.event_id == i.approval_event_id) + .ok_or_else(|| { + MergeRequestError::NotReady( + "approval is not the current effective review for the source ref".into(), + ) + })?; + if review.decision != ReviewDecision::Approve { + return Err(MergeRequestError::NotReady( + "current effective review does not approve the source ref".into(), + )); + } + if i.target_ref_before == i.target_ref_after { + return Err(MergeRequestError::Validation( + "target ref did not change".into(), + )); + } + let state: Option = self + .lock()? + .query_row( + "SELECT workflow_state FROM typed_tickets WHERE workspace_id=?1 AND ticket_id=?2", + params![mr.workspace_id, i.ticket_id], + |row| row.get(0), + ) + .optional()?; + if state.as_deref() != Some("inprogress") { + return Err(MergeRequestError::Conflict( + "Ticket must be inprogress".into(), + )); + } + Ok(()) + } + pub fn complete(&self, i: CompleteMergeRequest) -> Result { + self.validate_completion(&i)?; let mr = self.get(&i.auth.workspace_id, &i.ticket_id)?; self.completion_auth(&i.auth, &i.ticket_id, &mr.repository_id)?; if let Some(v) = mr.thread.iter().find_map(|x| match x { @@ -765,12 +825,21 @@ impl MergeRequestStore { i: RepairSelectorFrom, ) -> Result { nonempty("selector_from", &i.selector_from)?; + nonempty("resolved_subject_ref", &i.resolved_subject_ref)?; let mr = self.get(&i.workspace_id, &i.ticket_id)?; if mr.selector_from.is_some() { return Err(MergeRequestError::Conflict( "selector_from is immutable after it is set".into(), )); } + let approved = mr + .effective_review(&i.resolved_subject_ref) + .is_some_and(|review| review.decision == ReviewDecision::Approve); + if !approved { + return Err(MergeRequestError::NotReady( + "selector repair must resolve to an approved thread subject".into(), + )); + } let mut c = self.lock()?; let t = c.transaction()?; let changed=t.execute("UPDATE merge_requests SET selector_from=?3,updated_at=?4 WHERE workspace_id=?1 AND merge_request_id=?2 AND selector_from IS NULL",params![mr.workspace_id,mr.merge_request_id,i.selector_from,i.now.to_rfc3339()])?; diff --git a/crates/merge-request/tests/store.rs b/crates/merge-request/tests/store.rs index 823a1cab..336df058 100644 --- a/crates/merge-request/tests/store.rs +++ b/crates/merge-request/tests/store.rs @@ -304,3 +304,51 @@ fn completion_cancels_outstanding_grants_and_late_submit_fails() { MergeRequestThreadEvent::ReviewCancelled(value) if value.reason.contains("completed before review submission")))); } + +#[test] +fn selector_repair_requires_and_accepts_an_approved_resolved_subject() { + let (dir, store) = fixture(); + open(&store); + approve(&store, "approved-subject", "approval"); + Connection::open(dir.path().join("db")).unwrap() + .execute("UPDATE merge_requests SET selector_from=NULL WHERE workspace_id='W' AND merge_request_id='MR'", []) + .unwrap(); + let repaired = store + .repair_selector_from(RepairSelectorFrom { + workspace_id: "W".into(), + ticket_id: "T".into(), + selector_from: "restored-work".into(), + resolved_subject_ref: "approved-subject".into(), + repaired_by: WorkerIdentity { + runtime_id: "browser".into(), + worker_id: "user".into(), + }, + reason: "confirmed migrated source".into(), + now: at(8), + }) + .unwrap(); + assert_eq!(repaired.selector_from.as_deref(), Some("restored-work")); +} + +#[test] +fn selector_repair_rejects_unapproved_resolved_subject() { + let (dir, store) = fixture(); + open(&store); + approve(&store, "approved-subject", "approval"); + Connection::open(dir.path().join("db")).unwrap() + .execute("UPDATE merge_requests SET selector_from=NULL WHERE workspace_id='W' AND merge_request_id='MR'", []) + .unwrap(); + let result = store.repair_selector_from(RepairSelectorFrom { + workspace_id: "W".into(), + ticket_id: "T".into(), + selector_from: "wrong-work".into(), + resolved_subject_ref: "different-subject".into(), + repaired_by: WorkerIdentity { + runtime_id: "browser".into(), + worker_id: "user".into(), + }, + reason: "wrong candidate".into(), + now: at(8), + }); + assert!(matches!(result, Err(MergeRequestError::NotReady(_)))); +} diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index c66d55c6..96b7ec8f 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -3877,14 +3877,17 @@ async fn scoped_repair_merge_request_selector( } let store = merge_request_store(&api, &workspace_id)?; let mr = store.get(&workspace_id, &ticket_id)?; - api.repository_reader() + let resolved_subject_ref = api + .repository_reader() .observe_merge_target(&mr.repository_id, Some(&input.selector_from)) - .map_err(repository_merge_evidence_error)?; + .map_err(repository_merge_evidence_error)? + .commit; Ok(Json(store.repair_selector_from( merge_request::RepairSelectorFrom { workspace_id, ticket_id, selector_from: input.selector_from, + resolved_subject_ref, repaired_by: merge_request::WorkerIdentity { runtime_id: "browser".into(), worker_id: "authenticated-user".into(), @@ -4079,17 +4082,6 @@ async fn scoped_complete_merge_request( ) .into()); } - let already = observed.commit == input.target_ref_after; - if !already { - repositories - .update_merge_target( - &mr.repository_id, - &mr.selector_to, - &input.target_ref_before, - &input.target_ref_after, - ) - .map_err(repository_merge_evidence_error)? - } let completion = merge_request::CompleteMergeRequest { ticket_id, operation_id: input.operation_id, @@ -4108,6 +4100,18 @@ async fn scoped_complete_merge_request( }, now: Utc::now(), }; + store.validate_completion(&completion)?; + let already = observed.commit == input.target_ref_after; + if !already { + repositories + .update_merge_target( + &mr.repository_id, + &mr.selector_to, + &input.target_ref_before, + &input.target_ref_after, + ) + .map_err(repository_merge_evidence_error)? + } match store.complete(completion) { Ok(v) => Ok(Json(v)), Err(e) => {