Allow to skip signature verification #664
Conversation
sig/line/bot/v2/webhook_parser.rbs
Outdated
| @skip_signature_verification: Proc? | ||
|
|
||
| def initialize: (channel_secret: String) -> void | ||
| def initialize: (channel_secret: String, ?skip_signature_verification: Proc?) -> void |
There was a problem hiding this comment.
Can we address this by writing like this, so as not to ignore in RBS? Type errors should be pointing out issues, so I'd like to avoid adding # steep:ignore as much as possible.
like
interface SkipSignatureValidationPredicate
def call: () -> bool
end
...
@skip_signature_verification: SkipSignatureValidationPredicate?There was a problem hiding this comment.
for reference, RBS itself defines this case like
https://github.com/ruby/rbs/blob/5f41fb581c824b6cd9b33e58dc4ca084d0eb0e02/core/proc.rbs#L359-L362
lib/line/bot/v2/webhook_parser.rb
Outdated
| # @param skip_signature_verification [Proc, nil] | ||
| # A function that determines whether to skip webhook signature verification. | ||
| # If the function returns true, the signature verification step is skipped. | ||
| # This can be useful in scenarios such as when you're in the process of updating | ||
| # the channel secret and need to temporarily bypass verification to avoid disruptions. |
There was a problem hiding this comment.
Probably it's more explicit if this document explains "the function must returns boolean value", though this already says "If the function returns true"?
|
Understood. I will also handle that issue, but for the sake of keeping this PR independent, I will address it in a follow-up PR. |
| # @param skip_signature_verification [() -> bool, nil] | ||
| # A callable object with type `() -> bool` that determines whether to skip | ||
| # webhook signature verification. Signature verification is skipped if and | ||
| # only if this callable is provided and returns `true`. | ||
| # This can be useful in scenarios such as when you're in the process of | ||
| # updating the channel secret and need to temporarily bypass verification | ||
| # to avoid disruptions. | ||
| def initialize(channel_secret:, skip_signature_verification: nil) |
There was a problem hiding this comment.
It would be better if skip_signature_verification accepts not only Procs but also boolean values. It is possible to pass Procs like -> { true } or -> { false } but just checking boolean (without calling) would be more efficient.
for example:
def should_skip_verification?
if @skip_signature_verification.respond_to?(:call)
@skip_signature_verification.call
else
@skip_signature_verification
end
endThere was a problem hiding this comment.
Thank you very much for your suggestion. Allowing boolean values could certainly be convenient in some cases. However, having both ()-> bool and bool in the same field would increase type complexity.
This feature is primarily intended for use cases where the decision to skip signature verification may change dynamically—for example, when controlled by environment variables.
Since the same behavior can be achieved by passing a function that simply returns a fixed true or false, we have decided not to adopt direct boolean support at this time.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Changes
Motivation
The signature returned with webhooks is calculated using a single channel secret. If the bot owner changes their channel secret, the signature for webhooks starts being calculated using the new channel secret. To avoid signature verification failures, the bot owner must update the channel secret on their server, which is used for signature verification. However, if there is a timing mismatch in the update—and such a mismatch is almost unavoidable—verification will fail during that period.
In such cases, having an option to skip signature verification for webhooks would be a convenient way to avoid these issues.