Skip to content

Conversation

habara-k
Copy link
Contributor

Changes

  • Allow skipping signature verification for webhooks

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.

@habara-k habara-k force-pushed the allow-to-skip-signature-verification branch from 43a6658 to e276dfc Compare September 19, 2025 09:34
@habara-k habara-k force-pushed the allow-to-skip-signature-verification branch from d79e580 to 47bd3ec Compare September 25, 2025 02:31
@habara-k habara-k force-pushed the allow-to-skip-signature-verification branch from 47bd3ec to e6a1849 Compare September 25, 2025 02:33
@habara-k habara-k requested a review from a team September 25, 2025 02:44
* 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.
*/
public $skipSignatureValidation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using a callable instead of a bool so that the user can dynamically skip validation? If so, it might be helpful to mention that briefly in the PHPDoc.

class EventRequestOptions
{
/**
* @var callable|null Function that returns boolean to determine if signature validation should be skipped.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to explicitly specify that it returns a bool.

Suggested change
* @var callable|null Function that returns boolean to determine if signature validation should be skipped.
* @var callable(): bool|null Function that returns boolean to determine if signature validation should be skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion!
Yes, the intention is to allow dynamic control — the callable can return a boolean value to determine whether signature validation should be skipped.

However, we had to simplify the PHPDoc type to callable|null instead of callable(): bool|null, because phpDocumentor 3.x currently doesn’t support the newer callable syntax with return types.
To avoid breaking the documentation build, we decided to keep the simpler type declaration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

https://docs.phpdoc.org/guide/references/phpdoc/types.html#:~:text=null%3B%0A%7D-,callable,-the%20element%20to

It says it's available from version 3.3.0, so is that not correct? We use 3.3.1

wget https://github.com/phpDocumentor/phpDocumentor/releases/download/v3.3.1/phpDocumentor.phar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and more, this is probably (callable(): bool)|null ...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or null|callable(): bool?

/**
* Constructor
*
* @param callable|null $skipSignatureValidation Function that returns boolean to determine if signature validation should be skipped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Suggested change
* @param callable|null $skipSignatureValidation Function that returns boolean to determine if signature validation should be skipped
* @param callable(): bool|null $skipSignatureValidation Function that returns boolean to determine if signature validation should be skipped

echo "DIFF_IS_EMPTY=$([[ -z "$diff_excluding_submodule" ]] && echo 'true' || echo 'false')" >> $GITHUB_ENV
echo "CURRENT_DATETIME=$(date +'%Y%m%d%H%M%S')" >> $GITHUB_ENV
# Save full diff to file and upload artifact
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognize this as a workaround, but do you encounter a diff when executing the steps written in the workflow locally?

# Install openapi-generator-cli
- run: echo "OPENAPI_GENERATOR_VERSION=7.11.0" >> $GITHUB_ENV
- uses: actions/cache@0400d5f644dc74513175e3cd8d07132dd4860809 # v4.2.4
id: openapi-generator-cache
env:
cache-name: openapi-generator-cache
with:
path: ~/bin/openapitools
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.OPENAPI_GENERATOR_VERSION }}
- if: steps.openapi-generator-cache.outputs.cache-hit != 'true'
run: |
mkdir -p ~/bin/openapitools
curl https://raw.githubusercontent.com/OpenAPITools/openapi-generator/master/bin/utils/openapi-generator-cli.sh > ~/bin/openapitools/openapi-generator-cli
chmod u+x ~/bin/openapitools/openapi-generator-cli
export PATH=$PATH:~/bin/openapitools/
OPENAPI_GENERATOR_VERSION=${{ env.OPENAPI_GENERATOR_VERSION }} openapi-generator-cli version
- name: Generate codes
run: |
export PATH=$PATH:~/bin/openapitools/
bash tools/gen-oas-client.sh
- name: Update document
run: |
wget https://github.com/phpDocumentor/phpDocumentor/releases/download/v3.3.1/phpDocumentor.phar
php phpDocumentor.phar run -d src -t docs

If so, this might confuse external contributors too like you, and we should consider measures to ensure stricter version control. For example, https://github.com/line/line-bot-sdk-php/blob/master/CONTRIBUTING.md should be updated.

.... However, since CI provides stable results, could it be that your local version is misaligned...?
@eucyt might help you when you're confused with something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the PHP version I was running locally was outdated. The issue is now resolved, but I plan to keep this change for future troubleshooting.

@habara-k habara-k self-assigned this Oct 7, 2025
@habara-k habara-k requested review from Yang-33 and eucyt October 8, 2025 02:34
Copy link
Contributor

@Yang-33 Yang-33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're waiting response! #741 (comment)

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