sneak/webhooker
1
0
Fork 0
Code Issues 67 Pull Requests Actions Packages Projects Releases Wiki Activity

Independent code audit #303

New Issue
Open
opened 2026-08-30 03:48:37 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-30 03:48:37 +02:00
Collaborator
Copy Link

Full read of main @ 4f5ecb1. One line per finding, grouped by severity.

Unfit for purpose

  • SQLite is opened with cache=shared, no busy_timeout, no WAL, and an unbounded database/sql pool (internal/database/database.go:140, internal/database/webhook_db_manager.go:237); the ingest handler's write transaction (internal/handlers/webhook.go:223) races engine workers writing results/status to the same per-webhook file (internal/delivery/engine.go:761, internal/delivery/engine.go:778), so under any concurrency you get immediate SQLITE_BUSY/SQLITE_LOCKED → 500s to senders and deliveries stuck pending (the write errors are only logged). No test exercises concurrent writers.
  • Notify drops tasks when the channel is full and the only recovery for pending rows is process restart (internal/delivery/engine.go:174-181); the periodic sweep handles retrying only (internal/delivery/engine.go:572-602), so "guaranteed delivery" is not true without a restart.

Significant

  • Circuit breaker half-open state busy-loops: Allow() returns false but CooldownRemaining() returns 0 (internal/delivery/circuit_breaker.go:71-73, :85-87), so circuitBreakerBlock reschedules with delay 0 (internal/delivery/target_http.go:145-165) and every queued task for that target spins through retryCh with a DB status write per iteration for the whole probe duration (up to 30s).
  • Login/password-change rate limiting keys on True-Client-IP/X-Real-IP/X-Forwarded-For unconditionally (internal/middleware/ratelimit.go:75), so the limit is bypassed by setting a header; each attempt costs a 64 MB argon2 hash (internal/database/password.go:18), which makes /pages/login a trivial memory-exhaustion DoS. Violates REPO_POLICIES "never trust X-Forwarded-For unconditionally".
  • WEBHOOKER_ENVIRONMENT defaults to dev (internal/config/config.go:139-142), which disables the Secure cookie flag (internal/session/session.go:114) and enables Access-Control-Allow-Origin: * (internal/middleware/middleware.go:143); an operator who forgets the variable ships an insecure prod. Default should be prod.
  • Listener bind failure only cancels the server's own context (internal/server/http.go:44-50); nothing calls fx.Shutdowner, so the process stays up with no listener instead of exiting non-zero.
  • Sessions are stateless HMAC cookies with no server-side revocation: password change does not invalidate other sessions (internal/handlers/profile.go:116), logout is client-side only (internal/handlers/auth.go:206), and a stolen cookie is valid for 7 days (internal/session/session.go:112).
  • SSRF blocking has no opt-out (internal/delivery/ssrf.go:41-59, enforced at creation internal/handlers/source_management.go:948 and at dial internal/delivery/ssrf.go:206); a self-hosted dispatcher that cannot deliver to RFC1918/LAN targets is missing its primary use case. Needs an allowlist (e.g. ALLOWED_TARGET_CIDRS).
  • Replay/redelivery does not exist despite being a headline README promise (README.md:174); the UI also shows no per-attempt results (status code, error, response body, duration) — templates/source_logs.html:26-30 renders only the delivery status string, so the "full request/response logging" claim is unobservable.

Minor

  • X-Forwarded-Proto is trusted from any client (internal/middleware/csrf.go:19, internal/handlers/source_management.go:314).
  • "Retain forever" (RetentionDays <= 0, internal/database/retention.go:143) is unreachable: create/edit parsers ignore v <= 0 (internal/handlers/source_management.go:162, :457) and the forms use min="1" (templates/source_edit.html:31).
  • Engine stop cancels the context used by in-flight requests (internal/delivery/engine.go:239, internal/delivery/target_http.go:358), so a shutdown mid-request records a failed attempt and burns a retry; time.AfterFunc retry timers are never cancelled and outlive the engine (internal/delivery/engine.go:199).
  • Backoff is uncapped 2^(n-1)s up to 2^30 s (internal/delivery/target_http.go:266-271); the form allows max_retries=20 (templates/source_detail.html:110), i.e. a 6-day gap before the last attempt.
  • attemptNum >= maxRetries (internal/delivery/target_http.go:178) makes max_retries=N mean N total attempts, not N retries; README/UI say retries.
  • Webhook delete ignores the errors of the child deletes (internal/handlers/source_management.go:513-521), does not remove archive-{id}.db (internal/database/webhook_db_manager.go:166), leaves a stale archiveWriter cached (internal/delivery/target_database.go:130), and closes the sql.DB that engine workers may still hold (internal/database/webhook_db_manager.go:155-162).
  • The request logger records the full URL of every request at Info (internal/middleware/middleware.go:125) and the webhook handler logs the entrypoint UUID again (internal/handlers/webhook.go:42-46); since the UUID is the credential, logs carry all credentials.
  • Per-webhook sql.DB pools are never bounded or closed; fds scale with webhook count, and the retention sweep opens every webhook DB hourly and never releases (internal/database/webhook_db_manager.go:241, internal/database/retention.go:163).
  • No indexes on deliveries.status, deliveries.event_id, delivery_results.delivery_id, events.created_at (internal/database/model_delivery.go:18-20, internal/database/model_delivery_result.go:7, internal/database/model_event.go:7); recovery, sweeps, retention and pagination are full scans.
  • reapExpired runs three deletes outside a transaction (internal/database/retention.go:204-252) and can delete events whose deliveries are in flight.
  • Session cookies are signed but not encrypted (sessions.NewCookieStore(keyBytes) with one key, internal/session/session.go:107) while README claims encrypted cookies (README.md:210).
  • /metrics is registered when only METRICS_USERNAME is set (internal/server/routes.go:73); an empty METRICS_PASSWORD is accepted as the credential (internal/middleware/middleware.go:224).
  • Login leaks user existence via timing: unknown users skip the argon2 hash (internal/handlers/auth.go:105-117).
  • HTTP target forwards every inbound header except hop-by-hop, including Cookie/Authorization (internal/delivery/target_http.go:444-454), and always POSTs regardless of the captured method (internal/delivery/target_http.go:360); User-Agent is hardcoded webhooker/1.0 (:489) rather than the build version.
  • Captured events omit the source IP, query string and path (internal/handlers/webhook.go:282-289), which replay/audit will need.
  • SSRF blocklist misses ::/128, 64:ff9b::/96 (NAT64), ff00::/8, 2001:db8::/32 (internal/delivery/ssrf.go:41-59); dial uses only ips[0] with no fallback (:220).
  • gorm.io/driver/sqlite drags in mattn/go-sqlite3 and forces CGO + a Debian builder (go.mod:20, Dockerfile:3, Dockerfile:51) while modernc is what runs; github.com/glebarez/sqlite removes CGO entirely. github.com/go-chi/chi v1.5.5 (go.mod:8) is the unmaintained pre-v5 line.
  • script/test always runs -v and omits -cover (script/test:9), contrary to the conditional-verbose-rerun pattern in REPO_POLICIES.
  • Dead code/stubs: APIKey model (internal/database/model_apikey.go), Target.MaxQueueSize (internal/database/model_target.go:29), empty /api/v1 route group (internal/server/routes.go:63), no-op OnStart (internal/handlers/handlers.go:112-116), Server.exitCode/cleanupForExit/MaintenanceMode (internal/server/server.go:100, :158).
  • Doc drift: README says /webhook/{uuid} accepts ANY method (README.md:713) but the handler is POST-only (internal/handlers/webhook.go:24); README Docker section says golang:1.24 (README.md:920); TODO.md "Next Step" is retention cleanup, already shipped in #78 (TODO.md:21), and lists password change as future, shipped in #83 (TODO.md:75).
Full read of `main` @ 4f5ecb1. One line per finding, grouped by severity. ## Unfit for purpose - SQLite is opened with `cache=shared`, no `busy_timeout`, no WAL, and an unbounded `database/sql` pool (`internal/database/database.go:140`, `internal/database/webhook_db_manager.go:237`); the ingest handler's write transaction (`internal/handlers/webhook.go:223`) races engine workers writing results/status to the same per-webhook file (`internal/delivery/engine.go:761`, `internal/delivery/engine.go:778`), so under any concurrency you get immediate `SQLITE_BUSY`/`SQLITE_LOCKED` → 500s to senders and deliveries stuck `pending` (the write errors are only logged). No test exercises concurrent writers. - `Notify` drops tasks when the channel is full and the only recovery for `pending` rows is process restart (`internal/delivery/engine.go:174-181`); the periodic sweep handles `retrying` only (`internal/delivery/engine.go:572-602`), so "guaranteed delivery" is not true without a restart. ## Significant - Circuit breaker half-open state busy-loops: `Allow()` returns false but `CooldownRemaining()` returns 0 (`internal/delivery/circuit_breaker.go:71-73`, `:85-87`), so `circuitBreakerBlock` reschedules with delay 0 (`internal/delivery/target_http.go:145-165`) and every queued task for that target spins through `retryCh` with a DB status write per iteration for the whole probe duration (up to 30s). - Login/password-change rate limiting keys on `True-Client-IP`/`X-Real-IP`/`X-Forwarded-For` unconditionally (`internal/middleware/ratelimit.go:75`), so the limit is bypassed by setting a header; each attempt costs a 64 MB argon2 hash (`internal/database/password.go:18`), which makes `/pages/login` a trivial memory-exhaustion DoS. Violates REPO_POLICIES "never trust X-Forwarded-For unconditionally". - `WEBHOOKER_ENVIRONMENT` defaults to `dev` (`internal/config/config.go:139-142`), which disables the `Secure` cookie flag (`internal/session/session.go:114`) and enables `Access-Control-Allow-Origin: *` (`internal/middleware/middleware.go:143`); an operator who forgets the variable ships an insecure prod. Default should be prod. - Listener bind failure only cancels the server's own context (`internal/server/http.go:44-50`); nothing calls `fx.Shutdowner`, so the process stays up with no listener instead of exiting non-zero. - Sessions are stateless HMAC cookies with no server-side revocation: password change does not invalidate other sessions (`internal/handlers/profile.go:116`), logout is client-side only (`internal/handlers/auth.go:206`), and a stolen cookie is valid for 7 days (`internal/session/session.go:112`). - SSRF blocking has no opt-out (`internal/delivery/ssrf.go:41-59`, enforced at creation `internal/handlers/source_management.go:948` and at dial `internal/delivery/ssrf.go:206`); a self-hosted dispatcher that cannot deliver to RFC1918/LAN targets is missing its primary use case. Needs an allowlist (e.g. `ALLOWED_TARGET_CIDRS`). - Replay/redelivery does not exist despite being a headline README promise (`README.md:174`); the UI also shows no per-attempt results (status code, error, response body, duration) — `templates/source_logs.html:26-30` renders only the delivery status string, so the "full request/response logging" claim is unobservable. ## Minor - `X-Forwarded-Proto` is trusted from any client (`internal/middleware/csrf.go:19`, `internal/handlers/source_management.go:314`). - "Retain forever" (`RetentionDays <= 0`, `internal/database/retention.go:143`) is unreachable: create/edit parsers ignore `v <= 0` (`internal/handlers/source_management.go:162`, `:457`) and the forms use `min="1"` (`templates/source_edit.html:31`). - Engine stop cancels the context used by in-flight requests (`internal/delivery/engine.go:239`, `internal/delivery/target_http.go:358`), so a shutdown mid-request records a failed attempt and burns a retry; `time.AfterFunc` retry timers are never cancelled and outlive the engine (`internal/delivery/engine.go:199`). - Backoff is uncapped `2^(n-1)s` up to `2^30 s` (`internal/delivery/target_http.go:266-271`); the form allows `max_retries=20` (`templates/source_detail.html:110`), i.e. a 6-day gap before the last attempt. - `attemptNum >= maxRetries` (`internal/delivery/target_http.go:178`) makes `max_retries=N` mean N total attempts, not N retries; README/UI say retries. - Webhook delete ignores the errors of the child deletes (`internal/handlers/source_management.go:513-521`), does not remove `archive-{id}.db` (`internal/database/webhook_db_manager.go:166`), leaves a stale `archiveWriter` cached (`internal/delivery/target_database.go:130`), and closes the `sql.DB` that engine workers may still hold (`internal/database/webhook_db_manager.go:155-162`). - The request logger records the full URL of every request at Info (`internal/middleware/middleware.go:125`) and the webhook handler logs the entrypoint UUID again (`internal/handlers/webhook.go:42-46`); since the UUID is the credential, logs carry all credentials. - Per-webhook `sql.DB` pools are never bounded or closed; fds scale with webhook count, and the retention sweep opens every webhook DB hourly and never releases (`internal/database/webhook_db_manager.go:241`, `internal/database/retention.go:163`). - No indexes on `deliveries.status`, `deliveries.event_id`, `delivery_results.delivery_id`, `events.created_at` (`internal/database/model_delivery.go:18-20`, `internal/database/model_delivery_result.go:7`, `internal/database/model_event.go:7`); recovery, sweeps, retention and pagination are full scans. - `reapExpired` runs three deletes outside a transaction (`internal/database/retention.go:204-252`) and can delete events whose deliveries are in flight. - Session cookies are signed but not encrypted (`sessions.NewCookieStore(keyBytes)` with one key, `internal/session/session.go:107`) while README claims encrypted cookies (`README.md:210`). - `/metrics` is registered when only `METRICS_USERNAME` is set (`internal/server/routes.go:73`); an empty `METRICS_PASSWORD` is accepted as the credential (`internal/middleware/middleware.go:224`). - Login leaks user existence via timing: unknown users skip the argon2 hash (`internal/handlers/auth.go:105-117`). - HTTP target forwards every inbound header except hop-by-hop, including `Cookie`/`Authorization` (`internal/delivery/target_http.go:444-454`), and always POSTs regardless of the captured method (`internal/delivery/target_http.go:360`); `User-Agent` is hardcoded `webhooker/1.0` (`:489`) rather than the build version. - Captured events omit the source IP, query string and path (`internal/handlers/webhook.go:282-289`), which replay/audit will need. - SSRF blocklist misses `::/128`, `64:ff9b::/96` (NAT64), `ff00::/8`, `2001:db8::/32` (`internal/delivery/ssrf.go:41-59`); dial uses only `ips[0]` with no fallback (`:220`). - `gorm.io/driver/sqlite` drags in `mattn/go-sqlite3` and forces CGO + a Debian builder (`go.mod:20`, `Dockerfile:3`, `Dockerfile:51`) while `modernc` is what runs; `github.com/glebarez/sqlite` removes CGO entirely. `github.com/go-chi/chi v1.5.5` (`go.mod:8`) is the unmaintained pre-v5 line. - `script/test` always runs `-v` and omits `-cover` (`script/test:9`), contrary to the conditional-verbose-rerun pattern in REPO_POLICIES. - Dead code/stubs: `APIKey` model (`internal/database/model_apikey.go`), `Target.MaxQueueSize` (`internal/database/model_target.go:29`), empty `/api/v1` route group (`internal/server/routes.go:63`), no-op `OnStart` (`internal/handlers/handlers.go:112-116`), `Server.exitCode`/`cleanupForExit`/`MaintenanceMode` (`internal/server/server.go:100`, `:158`). - Doc drift: README says `/webhook/{uuid}` accepts `ANY` method (`README.md:713`) but the handler is POST-only (`internal/handlers/webhook.go:24`); README Docker section says `golang:1.24` (`README.md:920`); `TODO.md` "Next Step" is retention cleanup, already shipped in #78 (`TODO.md:21`), and lists password change as future, shipped in #83 (`TODO.md:75`).
clawbot commented 2026-08-30 03:57:42 +02:00
Author
Collaborator
Copy Link

Filed as individual issues: #304 #305 (unfit for purpose), #306 #307 #308 #309 #310 (significant). Skipped per sneak: login rate-limit header spoofing / argon2 DoS and session invalidation/revocation — single admin user with a long random password; good enough for now. Minor findings remain here untracked.

Filed as individual issues: #304 #305 (unfit for purpose), #306 #307 #308 #309 #310 (significant). Skipped per sneak: login rate-limit header spoofing / argon2 DoS and session invalidation/revocation — single admin user with a long random password; good enough for now. Minor findings remain here untracked.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#303
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?