done with this issue#91
Conversation
|
@chideraisiguzor Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
| ContractID string `json:"contractId"` | ||
| Topic0 *string `json:"topic0,omitempty"` | ||
| TargetURL string `json:"targetUrl"` | ||
| Secret string `json:"secret,omitempty"` |
There was a problem hiding this comment.
Secret is included in list responses. GET /v1/webhooks scans the secret from the DB into this struct and writeJSON serialises it. Issue #78 states the secret is shown once at creation time and is not retrievable after that. Remove Secret from GET /v1/webhooks and GET /v1/webhooks/{id} responses — zero it out or use a separate response type for reads.
| } | ||
| } | ||
| var id string | ||
| if err := db.QueryRowContext(ctx, `INSERT INTO api_keys DEFAULT VALUES RETURNING id`).Scan(&id); err != nil { |
There was a problem hiding this comment.
When no X-API-Key header is present, resolveAPIKeyID silently inserts a new row into api_keys and returns the fresh ID. Any unauthenticated caller can create webhook subscriptions this way — the endpoint has no real authentication. Unauthenticated requests should return 401 Unauthorized, not an implicitly-minted API key.
| } | ||
| } | ||
|
|
||
| func deleteWebhookHandler(db *sql.DB) http.HandlerFunc { |
There was a problem hiding this comment.
deleteWebhookHandler (and pauseWebhookHandler/resumeWebhookHandler) execute on id alone with no ownership check. Any caller who knows a subscription UUID can delete or pause a subscription belonging to another API key. Add a WHERE id = $1 AND api_key_id = $2 clause and resolve the caller's API key from the request header before executing the mutation.
| } | ||
| } | ||
|
|
||
| func generateWebhookSecret() string { |
There was a problem hiding this comment.
fmt.Sprintf("whsec_%d", time.Now().UnixNano()) is not cryptographically random. The timestamp is predictable, especially on systems where the clock is observable. Replace with crypto/rand:
import "crypto/rand"
import "encoding/hex"
func generateWebhookSecret() string {
b := make([]byte, 32)
if _, err := rand.Read(b); err != nil {
panic(err)
}
return "whsec_" + hex.EncodeToString(b)
}| // mux.HandleFunc("GET /v1/events", handlers.ListEvents) | ||
| // mux.HandleFunc("GET /v1/events/{id}", handlers.GetEvent) | ||
| // --------------------------------------------------------------------------- | ||
| mux.HandleFunc("GET /v1/webhooks", listWebhooksHandler(db)) |
There was a problem hiding this comment.
db is nil when newDB() fails (e.g. DATABASE_URL unset). These handlers get a nil *sql.DB and will panic on first request. Either nil-check db inside each handler and return 503, or exit at startup if the DB is required.
|
|
||
| go func() { | ||
| for { | ||
| entries, err := redisClient.XReadGroup(ctx, &redis.XReadGroupArgs{ |
There was a problem hiding this comment.
XReadGroup requires the consumer group to already exist. Calling it against a group that was never created returns NOGROUP No such consumer group. Add XGroupCreateMkStream before this loop to create-if-not-exists: redisClient.XGroupCreateMkStream(ctx, streamKey, groupName, "0")
| @@ -0,0 +1,590 @@ | |||
| package main | |||
There was a problem hiding this comment.
No tests for this 590-line file. Needed: (1) unit tests for signWebhookBody/verifyWebhookSignature (security-critical HMAC); (2) handler tests with httptest for createWebhookHandler and listWebhooksHandler; (3) processWebhookEvent logic test. Also: resolveAPIKeyID auto-creates an api_keys row for any request with an unknown or absent X-API-Key — this is effectively unauthenticated access to all webhook CRUD endpoints. Add auth or at minimum document the limitation. Also: this PR targets the fork main branch; use a feature branch (feat/webhooks) to avoid the diff moving with future commits.
| @@ -0,0 +1,32 @@ | |||
| CREATE TABLE IF NOT EXISTS api_keys ( | |||
There was a problem hiding this comment.
api_keys has only id and created_at — no key_hash or credential column. resolveAPIKeyID in webhooks.go has nothing to validate against; any caller can supply any UUID and bypass auth. Add key_hash TEXT NOT NULL (store a SHA-256 or bcrypt hash of the raw key, never the plaintext).
| @@ -0,0 +1,590 @@ | |||
| package main | |||
There was a problem hiding this comment.
Subscription CRUD is present but the webhook dispatcher is missing — nothing reads incoming events and POSTs to target_url. Issue #78 requires events to be delivered, not just subscriptions to be managed. The dispatcher loop, retry logic with exponential backoff, and webhook_deliveries insert must be implemented before this PR can merge.
| // --------------------------------------------------------------------------- | ||
| mux.HandleFunc("GET /v1/webhooks", listWebhooksHandler(db)) | ||
| mux.HandleFunc("POST /v1/webhooks", createWebhookHandler(db)) | ||
| mux.HandleFunc("DELETE /v1/webhooks/{id}", deleteWebhookHandler(db)) |
There was a problem hiding this comment.
PR was submitted from the contributor's main branch. Per project convention all branches come off dev and must be named type/issue-number-short-description (e.g. feat/78-webhooks). Resubmit from a feature branch — submitting from main risks diverging your fork's history from upstream.
|
|
||
| go 1.22 | ||
| go 1.25.0 | ||
|
|
There was a problem hiding this comment.
go 1.25.0 is not a valid Go release. The latest stable version is Go 1.23.x. Set this to a real version, e.g. go 1.23. This will cause go mod download and CI toolchain checks to fail.
| @@ -1,3 +1,18 @@ | |||
| module github.com/Depo-dev/trident/services/api | |||
|
|
|||
There was a problem hiding this comment.
This PR is submitted from the main branch of your fork. Any future commits you push to your fork's main will automatically appear in this PR. Please push your changes to a feature branch (e.g. feat/78-webhook-delivery) and retarget the PR from that branch. This is also required by CONTRIBUTING.md: "All branches come off dev — main is for tagged releases only."
| @@ -0,0 +1,32 @@ | |||
| CREATE TABLE IF NOT EXISTS api_keys ( | |||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now() | |||
There was a problem hiding this comment.
The api_keys table created here has only id and created_at — there is no column to store the actual API key value or hash. The webhook_subscriptions table references api_key_id, but resolveAPIKeyID in webhooks.go has no way to look up a key by its value since the value is never stored. Add a key_hash TEXT NOT NULL UNIQUE column (store a SHA-256 or bcrypt hash of the key, never the plaintext) so the auth lookup can work.
| continue | ||
| } | ||
| if err := processWebhookEvent(ctx, db, event); err != nil { | ||
| slog.Error("webhook delivery failed", "err", err) |
There was a problem hiding this comment.
deleteWebhookHandler does not verify that the subscription being deleted belongs to the authenticated API key — it deletes by UUID alone. A user who knows another user's webhook ID can silently delete it. Add an ownership check:
DELETE FROM webhook_subscriptions WHERE id = $1 AND api_key_id = $2and return 404 (not 403) if no rows are affected, to avoid leaking whether the ID exists.
| @@ -0,0 +1,590 @@ | |||
| package main | |||
There was a problem hiding this comment.
No tests are included. Per issue #78 acceptance criteria: "Integration test: create subscription, publish event, assert webhook received with correct payload and signature." At minimum, add handler-level tests using net/http/httptest covering create, list, delete, pause/resume, and deliveries endpoints.
| @@ -0,0 +1,32 @@ | |||
| CREATE TABLE IF NOT EXISTS api_keys ( | |||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now() | |||
There was a problem hiding this comment.
The api_keys table created here (only id + created_at) conflicts with the already-merged PR #169 which defines the full api_keys schema (key_hash, key_prefix, label, network, rate_limit_tier, last_used_at, request_count). This migration will fail on any database where #169 has already been applied. Remove this CREATE TABLE IF NOT EXISTS api_keys block entirely — the table already exists. The webhook_subscriptions FK to api_keys(id) will still work.
|
Implementation looks good — webhooks CRUD, delivery worker, retry/backoff, HMAC signing, cleanup job all present. Ready to merge once you resolve the merge conflicts with main. Rebase your branch on main, resolve any conflicts, and force-push. |
| @@ -19,13 +19,28 @@ func main() { | |||
|
|
|||
There was a problem hiding this comment.
main.go is built on a stub scaffold (old base with commented-out handler stubs). It is missing all existing production handlers: /v1/events, /v1/events/{id}, /v1/api-keys, /v1/stats/indexer, GraphQL, WebSocket, auth middleware, rate limiting, and gRPC client setup. Rebase this branch on current main and wire the webhook handlers into the actual routing setup, not the scaffold.
closes #78
Context
The WebSocket subscription model works well for long-lived browser connections and backend services that can maintain a persistent connection. But many real-world use cases cannot hold an open WebSocket:
Serverless functions (AWS Lambda, Vercel Edge Functions, Cloudflare Workers) are ephemeral — they cannot maintain a persistent connection
Mobile apps on spotty network connections cannot rely on WebSocket staying open
Automation scripts and CI jobs that want to react to a contract event (e.g., trigger a test suite when a specific event is emitted) cannot run indefinitely
Integrations with other services (Discord bots, Notion databases, internal tools) are much easier to build with a webhook than with a WebSocket consumer
Webhooks solve this by inverting the subscription model: instead of the client pulling events, Trident pushes them.