Skip to content

Webhook server verifies the signature only when one is sent, so an unsigned forged payload is accepted #43

Description

@toolshedlabs-hash

Summary

On scrapfly-sdk 0.11.1, the built in webhook server verifies a signature when one is present
and skips verification entirely when the header is absent. A POST with a forged body and no
X-Scrapfly-Webhook-Signature header reaches the user's callback.

The same shape appears a second time, in crawler/crawler_webhook.py:372:

if signing_secrets and signature:
    ...
    if not handler.verify(message, signature):
        raise WebhookSignatureMissMatch()

so both entry points verify only when a signature happens to be supplied.

Reproduction

Clean venv, scrapfly-sdk==0.11.1 from PyPI plus the webhook-server extra, Python 3.14.0.
The server below is the shipped create_server, not a reimplementation.

from scrapfly.webhook import create_server

received = []
app = create_server(signing_secrets=("probe-signing-secret",),
                    callback=lambda data, kind, req: received.append(data))
# served on 127.0.0.1

Then three POSTs to /webhook with the same body and
X-Scrapfly-Webhook-Resource-Type: scrape:

signature header HTTP callback fired
absent 200 yes, payload delivered
present but wrong 500 no, WebhookSignatureMissMatch
present and correct 200 yes

The second row is what shows the verifier works, so the first row is about absence
specifically and not about verification being off everywhere. The body was the same each time,
{"result": {"content": "FORGED BY THE PROBE"}}. Row one delivered it verbatim.

Where it is

ResponseBodyHandler.read gates on the signature being non-None:

api_response.py:162    if self._signing_secret is not None and signature is not None:
api_response.py:163        if not self.verify(content, signature):
api_response.py:164            raise WebhookSignatureMissMatch()

and the route supplies it from a header that may not be there:

webhook.py:38    signature=headers.get('X-Scrapfly-Webhook-Signature', None) # Can be none when ping during the webhook creation flow via "ping"

The comment explains the intent, and the ping case is a real thing to handle. The cost is the
scope. It applies everywhere. Every resource type, every request, not just the creation time
ping.

Suggested fix

If a signing secret is configured, treat a missing signature as a failure:

if self._signing_secret is not None:
    if signature is None or not self.verify(content, signature):
        raise WebhookSignatureMissMatch()

That would reject the creation time ping too, so the exemption probably wants to be explicit
about the case it was written for, keyed off X-Scrapfly-Webhook-Resource-Type == 'ping' in
webhook.py rather than off the header being absent.

One smaller thing in the same function

api_response.py:139 compares with ==:

computed = hmac.new(signing_secret, message, hashlib.sha256).hexdigest().upper()
...
if computed == signature:

hmac.compare_digest(computed, signature) is the constant time version. Low priority next to
the above, and mentioned only because it is four lines away.

Happy to open a PR for either or both if that is useful.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions