-
Notifications
You must be signed in to change notification settings - Fork 29
fix(workflows): in-place updates, deletion-forgery fix, webhook-secret & channel immutability #1340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
67f8c53
e6956dd
9e34ed4
efa13ed
829cf10
4e5701f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -568,6 +568,13 @@ async fn handle_workflow_def( | |
| IngestError::Rejected("invalid: missing workflow name (name or d tag)".into()) | ||
| })?; | ||
|
|
||
| // Parse the d-tag as the workflow_id UUID | ||
| let d_tag = extract_d_tag(event) | ||
| .ok_or_else(|| IngestError::Rejected("invalid: missing d tag (workflow ID)".into()))?; | ||
| let workflow_id = Uuid::parse_str(&d_tag).map_err(|_| { | ||
| IngestError::Rejected("invalid: workflow ID in d tag must be a valid UUID".into()) | ||
| })?; | ||
|
|
||
| // 2. Validate caller has channel access (minimum: is a member) | ||
| let is_member = state | ||
| .is_member_cached(channel_id, &self_bytes) | ||
|
|
@@ -579,6 +586,31 @@ async fn handle_workflow_def( | |
| )); | ||
| } | ||
|
|
||
| // Check if workflow already exists to perform update or create checks | ||
| let existing = state.db.get_workflow(workflow_id).await; | ||
| let existing_record = match existing { | ||
| Ok(record) => { | ||
| if record.owner_pubkey != self_bytes { | ||
| return Err(IngestError::Rejected( | ||
| "forbidden: cannot update a workflow owned by another user".into(), | ||
| )); | ||
| } | ||
| if record.channel_id != Some(channel_id) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛓️ Channel immutability + legacy NULL. |
||
| return Err(IngestError::Rejected( | ||
| "forbidden: cannot change the channel of an existing workflow".into(), | ||
| )); | ||
| } | ||
| Some(record) | ||
| } | ||
| Err(buzz_db::DbError::NotFound(_)) => None, | ||
| Err(e) => { | ||
| return Err(IngestError::Internal(format!( | ||
| "error: db lookup workflow: {e}" | ||
| ))); | ||
| } | ||
| }; | ||
| let is_update = existing_record.is_some(); | ||
|
|
||
| // 3. Parse YAML from event.content | ||
| let (def, definition_json_str) = buzz_workflow::WorkflowEngine::parse_yaml(&event.content) | ||
| .map_err(|e| IngestError::Rejected(format!("invalid: workflow YAML parse error: {e}")))?; | ||
|
|
@@ -588,9 +620,17 @@ async fn handle_workflow_def( | |
|
|
||
| // Generate webhook secret if this workflow uses a Webhook trigger | ||
| let webhook_secret = if matches!(def.trigger, buzz_workflow::TriggerDef::Webhook) { | ||
| let secret = webhook_secret::generate_webhook_secret(); | ||
| webhook_secret::inject_secret(&mut definition_json, &secret); | ||
| Some(secret) | ||
| let existing_secret = existing_record | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔑 Webhook secret preservation. On update we re-inject the existing secret from the stored row (returning |
||
| .as_ref() | ||
| .and_then(|r| crate::webhook_secret::extract_secret(&r.definition)); | ||
| if let Some(secret) = existing_secret { | ||
| webhook_secret::inject_secret(&mut definition_json, &secret); | ||
| None | ||
| } else { | ||
| let secret = webhook_secret::generate_webhook_secret(); | ||
| webhook_secret::inject_secret(&mut definition_json, &secret); | ||
| Some(secret) | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
@@ -612,20 +652,29 @@ async fn handle_workflow_def( | |
| PersistResult::Inserted(tx) => tx, | ||
| }; | ||
|
|
||
| // 4. Execute: create_workflow | ||
| let workflow_id = state | ||
| .db | ||
| .create_workflow( | ||
| Some(channel_id), | ||
| &self_bytes, | ||
| &workflow_name, | ||
| &definition_json_final, | ||
| &hash, | ||
| ) | ||
| .await | ||
| .map_err(|e| IngestError::Internal(format!("error: db create_workflow: {e}")))?; | ||
| // 4. Execute: update_workflow or create_workflow | ||
| if is_update { | ||
| state | ||
| .db | ||
| .update_workflow(workflow_id, &workflow_name, &definition_json_final, &hash) | ||
| .await | ||
| .map_err(|e| IngestError::Internal(format!("error: db update_workflow: {e}")))?; | ||
|
Comment on lines
+679
to
+683
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an older kind:30620 event for the same workflow is delivered after a newer one (for example after a reconnect/retry, while still inside the relay's timestamp window), this unconditional Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Addressed in efa13ed. The command path now goes through the same NIP-33 replacement logic as the normal store path. Rather than duplicate the check, I extracted the lock +
👍 Useful catch — thanks. |
||
| } else { | ||
| state | ||
| .db | ||
| .create_workflow( | ||
| workflow_id, | ||
| Some(channel_id), | ||
| &self_bytes, | ||
| &workflow_name, | ||
| &definition_json_final, | ||
| &hash, | ||
| ) | ||
| .await | ||
| .map_err(|e| IngestError::Internal(format!("error: db create_workflow: {e}")))?; | ||
| } | ||
|
|
||
| // Commit: event + workflow creation succeeded atomically. | ||
| // Commit: event + workflow creation/update succeeded atomically. | ||
| tx.commit() | ||
| .await | ||
| .map_err(|e| IngestError::Internal(format!("error: commit transaction: {e}")))?; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1586,12 +1586,24 @@ async fn handle_a_tag_deletion(event: &Event, state: &Arc<AppState>) -> anyhow:: | |
| buzz_core::kind::KIND_WORKFLOW_DEF => { | ||
| // Try UUID first (workflow_id); fall back to name-based lookup. | ||
| if let Ok(wf_id) = uuid::Uuid::parse_str(d_tag) { | ||
| state | ||
| .db | ||
| .delete_workflow(wf_id) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("failed to delete workflow {wf_id}: {e}"))?; | ||
| tracing::info!(workflow_id = %wf_id, "Workflow deleted via NIP-09 a-tag (UUID)"); | ||
| let owner_bytes = hex::decode(pubkey_hex).unwrap_or_default(); | ||
| match state.db.get_workflow(wf_id).await { | ||
| Ok(wf) => { | ||
| if wf.owner_pubkey != owner_bytes { | ||
| return Err(anyhow::anyhow!("forbidden: deletion owner mismatch")); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security (deletion forgery). NIP-09 a-tag deletion previously only checked the actor matched the coordinate pubkey, then deleted by UUID. A forged |
||
| } | ||
| state.db.delete_workflow(wf_id).await.map_err(|e| { | ||
| anyhow::anyhow!("failed to delete workflow {wf_id}: {e}") | ||
| })?; | ||
| tracing::info!(workflow_id = %wf_id, "Workflow deleted via NIP-09 a-tag (UUID)"); | ||
| } | ||
| Err(buzz_db::DbError::NotFound(_)) => { | ||
| tracing::warn!("NIP-09 a-tag deletion: no workflow '{wf_id}' found"); | ||
| } | ||
| Err(e) => { | ||
| return Err(anyhow::anyhow!("failed to lookup workflow for delete: {e}")); | ||
| } | ||
| } | ||
| } else { | ||
| // Name-based lookup | ||
| let owner_bytes = hex::decode(pubkey_hex).unwrap_or_default(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 Ordering is load-bearing (info-leak). The
is_member_cachedgate above (line 580) runs before this lookup on purpose, so unauthorized users can't probe for valid workflow UUIDs via differing error responses. Please don't hoist this lookup earlier for efficiency — it silently reintroduces the leak.