refactor: trigger listener

[dieriba]

No description provided.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	b1e4aee
Status:	✅  Deploy successful!
Preview URL:	https://9316dccb.windmill.pages.dev
Branch Preview URL:	https://dieri-refactor-trigger.windmill.pages.dev

View logs

[claude]

Claude finished @dieriba's task —— View job

---

Comprehensive Code Review of Trigger Refactoring

This is a substantial architectural refactoring that consolidates trigger handling across multiple systems (HTTP, WebSocket, Kafka, MQTT, NATS, PostgreSQL, SQS, GCP) under a unified TriggerCrud trait interface. Overall, this is a well-executed refactoring that significantly improves code organization and maintainability.

Todo List:

• Read repository guidelines and understand codebase structure
• Analyze the scope of the trigger refactoring changes
• Review the TriggerCrud trait architecture and implementation patterns
• Analyze specific trigger implementations (HTTP, Kafka, MQTT, etc.)
• Check database schema changes and migrations
• Review error handling and security patterns
• Identify potential bugs, security issues, and performance concerns
• Provide comprehensive feedback with specific suggestions
• Create testing reproduction instructions

---

✅ Strengths

1. Excellent Architectural Design

The TriggerCrud trait provides a clean, unified interface that eliminates code duplication across trigger types. The generic design with associated types is well-structured:

trait TriggerCrud: Send + Sync + 'static {
    type Trigger: Serialize + DeserializeOwned + FromRow + Send + Sync + Unpin;
    type TriggerConfig: Debug + DeserializeOwned + FromRow + Serialize + Send + Sync + Unpin;
    // ...
}

2. Consistent Error Handling

The migration to unified error handling with consistent retry mechanisms across all trigger types (backend/migrations/20250707120256_trigger_retries_and_error_handling.up.sql) is excellent.

3. Security Implementation

The webhook authentication system (http_trigger_auth.rs) implements proper HMAC validation for multiple providers (GitHub, Slack, Stripe, Twitch) with constant-time comparison to prevent timing attacks.

4. Code Consolidation

The elimination of many redundant SQLx query files (100 files changed, 13533 deletions) shows effective deduplication.

---

⚠️ Areas of Concern

1. SQL Injection Risk - CRITICAL

File: backend/windmill-api/src/triggers/handler.rs:148-166

let sql = format!(
    "SELECT {} FROM {} WHERE workspace_id = $1 AND path = $2",
    fields.join(", "),
    Self::TABLE_NAME  // ⚠️ POTENTIAL SQL INJECTION
);

Issue: Self::TABLE_NAME is inserted directly into SQL without validation. While it's a constant, if any implementation provides an untrusted value, this could lead to SQL injection.

Recommendation: Use a whitelist validation or consider using a macro/proc-macro approach:

const VALID_TABLE_NAMES: &[&str] = &["http_trigger", "mqtt_trigger", /* ... */];
if !VALID_TABLE_NAMES.contains(&Self::TABLE_NAME) {
    return Err(Error::InternalErr("Invalid table name".to_string()));
}

2. Resource Leak Potential

File: backend/windmill-api/src/triggers/listener.rs:370

The listener patterns spawn long-running tasks but there's no clear cleanup mechanism visible for failed connections or disabled triggers. Consider implementing proper resource cleanup.

3. Error Message Information Disclosure

File: Multiple locations with format! calls

Several error messages might expose internal system details:

Error::NotFound(format!("Trigger not found at path: {}", path))

Recommendation: Use sanitized error messages in production.

4. Performance Concern - Query Generation

The dynamic SQL generation in get_trigger_by_path rebuilds the field list and SQL string on every call. Consider caching or using compile-time generation for frequently-called methods.

---

🔍 Detailed Issues & Suggestions

Database Queries

• Positive: Proper use of parameterized queries with .bind()
• Concern: Dynamic table name insertion in multiple locations could be more defensive

HMAC Implementation

• Positive: Uses constant-time comparison via hmac.verify()
• Positive: Proper encoding handling (Base64, Hex) for different providers
• Minor: Consider adding timestamp validation for replay attack prevention in Slack handler

Memory Management

• Positive: Good use of Arc for shared state
• Positive: Proper FromRow implementations avoid unnecessary allocations

Error Handling Pattern

• Positive: Consistent use of Result<T, Error> throughout
• Minor: Some error contexts could be more descriptive for debugging

---

🎯 Specific Recommendations

1. Security Hardening

// Add table name validation
fn validate_table_name(table: &str) -> Result<()> {
    const ALLOWED_TABLES: &[&str] = &["http_trigger", "mqtt_trigger", "kafka_trigger", /* ... */];
    if ALLOWED_TABLES.contains(&table) { Ok(()) } 
    else { Err(Error::BadRequest("Invalid table".to_string())) }
}

2. Performance Optimization

Cache the SQL queries since they're deterministic based on constants:

lazy_static! {
    static ref SQL_CACHE: HashMap<&'static str, String> = HashMap::new();
}

3. Observability Enhancement

Add structured logging with trigger type context:

tracing::info!(
    trigger_type = Self::TRIGGER_TYPE,
    workspace_id = workspace_id,
    "Creating trigger"
);

---

✅ Testing Instructions

To test this trigger refactoring, navigate to the Windmill web interface and verify the following functionality:

Trigger Management: Access the "Triggers" section in any workspace. Create, edit, and delete triggers of different types (HTTP routes, WebSocket endpoints, MQTT subscriptions) to ensure the unified CRUD operations work correctly. Test the new error handling and retry configuration options that were added to all trigger types.

HTTP Triggers: Create HTTP triggers with various authentication methods (webhooks from GitHub, Slack, Stripe). Verify route path validation, duplicate detection, and static website serving. Test both workspaced and global routes.

Error Handling: Configure triggers with error handlers and retry policies using the new unified error handling system. Trigger failures and verify that error handlers execute and retries work as expected.

Listener Status: Check that trigger listeners show proper connection status, error reporting, and auto-disable functionality when encountering persistent failures.

The refactoring should be transparent to end users while providing more consistent behavior across all trigger types.

---

[ellipsis-dev]

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]

---

Generated with ❤️ by ellipsis.dev