Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why is timestamp tolerance check in WebhookSignature one-sided? #657

Closed
CyberiaResurrection opened this issue May 20, 2019 · 2 comments · Fixed by #658
Closed

Why is timestamp tolerance check in WebhookSignature one-sided? #657

CyberiaResurrection opened this issue May 20, 2019 · 2 comments · Fixed by #658

Comments

@CyberiaResurrection
Copy link

CyberiaResurrection commented May 20, 2019

PHP version: 7.2.18
stripe-php version: v6.35.0

If this is deliberate, then fair enough, can a maintainer close this issue and make sure it's documented.

Line 63 of lib/WebhookSignature.php:

        // Check if timestamp is within tolerance
        if (($tolerance > 0) && ((time() - $timestamp) > $tolerance)) {
            throw new Error\SignatureVerification(
                "Timestamp outside the tolerance zone",
                $header,
                $payload
            );
        }

When time() is greater than supplied timestamp -
time() - 1558331514
timestamp - 1558331213

and default 300 second tolerance, that blows up because time() is 301 seconds greater than timestamp.

In light of the doc block for verifyHeader, namely

     * @param int $tolerance maximum difference allowed between the header's
     *  timestamp and the current time

Why, since tolerances tend to be two-sided in my experience, doesn't the timestamp check blow up when the values of time() and timestamp are reversed - ie, timestamp is more than 5 minutes ahead of time() ?

@ob-stripe
Copy link
Contributor

ob-stripe commented May 20, 2019

Hi @CyberiaResurrection, thanks for reporting this. This looks like an implementation error -- the check should probably be abs(time() - $timestamp) > $tolerance. We'll release a fix soon.

@ob-stripe
Copy link
Contributor

Fixed in 6.35.1.

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 a pull request may close this issue.

2 participants