bullet-train-co / bullet_train-core

The Open Source Ruby on Rails SaaS Framework
38 stars 45 forks source link

Continuous Delivery Attempt when `advanced_hostname_security` #739

Open phillipn opened 10 months ago

phillipn commented 10 months ago

If you were to enable the BulletTrain::OutgoingWebhooks.advanced_hostname_security (not enabled on prod atm), there is potential to hit an infinite loop of sorts.

Suppose that you attempt to call deliver on an outgoing delivery to an endpoint:

  def deliver
    # TODO If we ever do away with the `async: true` default for webhook generation, then I believe this needs to
    # change otherwise we'd be attempting the first delivery of webhooks inline.
    if delivery_attempts.create.attempt
      touch(:delivered_at)
    else
      deliver_async
    end
  end

It is important to note that delivery_attempts.create ALWAYS fails to create due to this validation error:

  validates :response_code, presence: true

So basically, we are initializing the record, and we rely on attempt to save the record.

If we hit this block in the delivery attempt, we will not save the record:

  if BulletTrain::OutgoingWebhooks.advanced_hostname_security
      unless allowed_uri?(uri)
        self.response_code = 0
        self.error_message = "URI is not allowed: " + uri
        return false
      end
    end

The end result, is that we will CONTINUOUSLY attempt to deliver the webhook via deliver_async, since we are never persisting the attempt itself. A delivery only stops attempting once Delivery#attempt_count, reaches a certain threshold.

jagthedrummer commented 8 months ago

@phillipn, is this still an issue, or did #740 resolve it?