Skip to content

Conversation

@Makisuo
Copy link
Collaborator

@Makisuo Makisuo commented Dec 13, 2025

Add isNull(deletedAt) filter to findById, updateLastUsed, updateToken, and softDelete methods in ChannelWebhookRepo. This ensures soft-deleted webhooks cannot be used to post messages, a security vulnerability that allowed deleted webhook tokens to remain functional indefinitely.

Add isNull(deletedAt) filter to findById, updateLastUsed, updateToken,
and softDelete methods in ChannelWebhookRepo. This ensures soft-deleted
webhooks cannot be used to post messages, a security vulnerability that
allowed deleted webhook tokens to remain functional indefinitely.
@claude
Copy link

claude bot commented Dec 13, 2025

Claude finished @Makisuo's task —— View job


PR Review: Fix soft-deleted webhooks being accessible

Analyzed the security fix for soft-deleted webhooks. This is a critical security patch that properly implements soft-delete filtering.

Todo List:

  • Read and analyze the modified file
  • Review code quality and implementation
  • Check for security concerns
  • Evaluate performance implications
  • Assess test coverage needs
  • Provide comprehensive feedback

Review Summary

✅ This is an excellent security fix that addresses a critical vulnerability where soft-deleted webhooks could still be accessed and used to post messages.

Code Quality & Implementation

Strengths:

  • Consistent pattern: The implementation follows the same soft-delete pattern used across other repositories (channel-member-repo.ts, organization-repo.ts)
  • Comprehensive coverage: All critical methods are properly updated (findById, updateLastUsed, updateToken, softDelete)
  • Proper query structure: Uses and() with isNull(deletedAt) consistently
  • Good documentation: Clear comments explaining the intent of each override
  • Type safety: Maintains full TypeScript safety with existing patterns

Implementation details:

  • Correctly overrides the base findById method from ModelRepository (/home/runner/work/hazel/hazel/apps/backend/src/repositories/channel-webhook-repo.ts:23-40)
  • Properly implements isNull(schema.channelWebhooksTable.deletedAt) filters in all affected methods
  • Returns expected types (Option<ChannelWebhook> for single records, arrays for lists)

Security Analysis

✅ Critical vulnerability fixed:

  • Before: Deleted webhooks with deletedAt !== null could still be found and used
  • After: All database queries properly filter out soft-deleted records
  • Impact: Prevents unauthorized message posting via "deleted" webhook tokens

Security best practices followed:

  • Database-level filtering ensures consistency
  • No bypass paths for soft-deleted records
  • Maintains audit trail (soft delete vs hard delete)

Performance Considerations

✅ Performance optimized:

  • Database schema includes channel_webhooks_deleted_at_idx index (/home/runner/work/hazel/hazel/packages/db/src/schema/channel-webhooks.ts:34)
  • isNull(deletedAt) queries will efficiently use this index
  • Query patterns follow existing codebase standards

Test Coverage Concerns

⚠️ Missing test coverage:

  • No unit tests found for ChannelWebhookRepo
  • Critical security fix should include tests verifying:
    // Recommended tests:
    // 1. Soft-deleted webhooks are not found by findById
    // 2. updateLastUsed/updateToken fail for soft-deleted webhooks  
    // 3. softDelete operation is idempotent
    // 4. findByTokenHash excludes soft-deleted webhooks

Minor Suggestions

  1. Consistency: Consider if baseRepo.findById should also be overridden in the return object, or if it's intentionally excluded
  2. Error handling: The current implementation returns empty results for deleted webhooks - consider if specific error types would be beneficial for debugging

Approval Recommendation

✅ APPROVE - This is a well-implemented security fix that:

  • Addresses a critical vulnerability
  • Follows established patterns
  • Maintains code quality
  • Has no breaking changes

Suggested follow-up: Add comprehensive unit tests to prevent regression of this security issue.


@github-actions
Copy link

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 85.11% 183 / 215
🔵 Statements 85.58% 196 / 229
🔵 Functions 97.14% 34 / 35
🔵 Branches 75.91% 104 / 137
File CoverageNo changed files found.
Generated in workflow #1011 for commit 6e61fdb by the Vitest Coverage Report Action

@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
hazel-app 6e61fdb Commit Preview URL

Branch Preview URL
Dec 13 2025, 01:08 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants