Repository navigation
chore(webhooks): withdrawn - #568
Closed
michael-chou359 wants to merge 1 commit into
Closed
michael-chou359 wants to merge 1 commit into
michael-chou359 wants to merge 1 commit into
Conversation
handle_webhook's docstring said the payload was trustworthy because the control plane's /agents/forward ingress had verified the GitHub/Slack HMAC. That holds only for requests that actually traversed the control plane. The route it documents is an ordinary FastAPI route on an app deployed with `uvicorn ... --host 0.0.0.0 --port 8000`, and FastACP adds no authentication to agent-defined routes — its x-agent-api-key check lives inside _handle_jsonrpc for POST /api alone. So any peer that could reach the pod port could POST an unsigned body and have it treated as a verified webhook (VULN-3137). That mattered twice over. session_key was an unkeyed SHA-1 of agent_name:channel:peer_id, and peer_id is `repo#number` taken from the body, so the task name of any real webhook conversation was computable from public data. task/create is get-or-create by name, so the caller joined the victim's task, had its sender_id/peer_id metadata overwritten from their own body, and — for sync agents, which load the full per-task state each turn — got a reply grounded in the victim PR's description, inlined diff and prior analysis, all on the operator's keys. Provenance is now mandatory and checked first: - New webhook_signatures.py verifies GitHub `sha256=` and Slack `v0:` HMACs over the RAW request bytes, with Slack's timestamp freshness (300s, both directions) and hmac.compare_digest. The control plane forwards the source's own signature headers (it strips only hop-by-hop ones), so the agent can re-verify them. - handle_webhook takes `verify=WebhookVerification(...)` and refuses to run without it, raising WebhookSignatureError — a distinct type so a handler answers 403 rather than 500. `trust_caller=True` is the explicit, named opt-out for a route that proved provenance some other way. - session_key is keyed with the webhook secret (HMAC-SHA256), so naming an existing conversation requires holding the secret, and two installations watching the same repository no longer collide. The documented pattern is rewritten to match, since copying the old one into an unauthenticated route was the whole defect: it now reads the raw body, passes WebhookVerification, and maps a signature failure to 403. Two consequences for existing callers, neither avoidable while failing closed: handle_webhook now raises unless given `verify` or `trust_caller`, and task names change (the digest is keyed, and SHA-1 became SHA-256), so an in-flight webhook conversation starts a fresh task. No shipped template or tutorial defines such a route, so nothing in-tree depends on the old behaviour. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+125
to
+126
| current = time.time() if now is None else now | ||
| if abs(current - timestamp) > max_age_seconds: |
There was a problem hiding this comment.
A 400-digit X-Slack-Request-Timestamp passes int() but raises OverflowError at current - timestamp. Non-ASCII signature values also make hmac.compare_digest() raise TypeError. The documented handler catches only WebhookSignatureError, so these requests get 500 rather than 403 and add misleading server failures to monitoring.
Check header formats or convert these exceptions to WebhookSignatureError, and add tests for both cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/sdk/utils/webhook_signatures.py
Line: 125-126
Comment:
**Malformed headers cause server errors**
A 400-digit `X-Slack-Request-Timestamp` passes `int()` but raises `OverflowError` at `current - timestamp`. Non-ASCII signature values also make `hmac.compare_digest()` raise `TypeError`. The documented handler catches only `WebhookSignatureError`, so these requests get 500 rather than 403 and add misleading server failures to monitoring.
Check header formats or convert these exceptions to `WebhookSignatureError`, and add tests for both cases.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+369
to
+376
| async def test_a_forged_signature_drives_no_turn(self): | ||
| raw = json.dumps(_pr_payload()).encode() | ||
| with pytest.raises(WebhookSignatureError, match="Invalid"): | ||
| await handle_webhook( | ||
| agent_name="golden-agent", | ||
| payload=_pr_payload(), | ||
| shaper="github_pr", | ||
| verify=WebhookVerification( |
There was a problem hiding this comment.
No test sends a valid WebhookVerification through handle_webhook(). Successful turn tests all use trust_caller=True, while the new tests check signatures and keyed names separately. Tests could stay green if valid signed requests stop working or use an unkeyed task name.
Add a successful signed request using the existing ADK mocks. Assert that create_task() receives the secret-keyed name and that the turn returns its reply.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/lib/test_webhooks.py
Line: 369-376
Comment:
**Signed turns lack success coverage**
No test sends a valid `WebhookVerification` through `handle_webhook()`. Successful turn tests all use `trust_caller=True`, while the new tests check signatures and keyed names separately. Tests could stay green if valid signed requests stop working or use an unkeyed task name.
Add a successful signed request using the existing ADK mocks. Assert that `create_task()` receives the secret-keyed name and that the turn returns its reply.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
michael-chou359
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Withdrawn — this change is being re-landed through the correct internal process.
Please disregard the previous contents of this pull request.