No in-app way to retrieve an event body larger than the event log's 8 KB render cap #157
Reference in New Issue
Block a user
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?
Follow-up to #135, whose definition of done asks for this to be filed if the untruncated body is not reachable.
The fix for #135 caps the event log page at 8192 bytes of body per event. Nothing is lost from storage: the full body stays in the per-webhook SQLite database, and the database archive target keeps its own full copy (
internal/delivery/target_database.go). But no HTTP route serves it —/api/v1is still an empty route group (internal/server/routes.go) — so a body over the cap is now reachable only out of band, by an operator with filesystem access to the data directory.Before the cap the event log page itself showed the whole body, so this is a gap the cap introduces for the app's own users.
Definition of done
GET /source/{sourceID}/logs/{eventID}/body, served with a non-renderable content type (application/octet-stream) andContent-Disposition: attachmentso an HTML or script payload cannot execute in the admin's origin.executeTemplate's buffering is not involved.Added to the
1.0.0milestone.Reason: #135's definition of done did permit filing this rather than fixing it inline, and #158 was right to take that option — but permitted is not the same as acceptable to ship. webhooker exists to durably store webhooks so they can be inspected and replayed. After the cap lands, a body over 8 KB cannot be seen through the product at all, only by an operator with filesystem access to the SQLite files. Debugging a large payload is precisely when the tool is supposed to earn its keep, so tagging 1.0 in that state means shipping a regression we introduced ourselves.
It is small and the definition of done above is already complete, so it costs little to close the gap in the same milestone rather than after it.
Two things the implementer must not drop, both already in the DoD and both load-bearing rather than stylistic: the response is attacker-controlled bytes being handed to the admin's own origin, so
application/octet-streamplusContent-Disposition: attachmentis what stops a stored HTML or script payload executing as the logged-in operator — this route would otherwise be a stored-XSS delivery mechanism. And it must stream from the row rather than buffer, or it reintroduces the resident-memory cost #135 just removed, on a route that serves the largest bodies in the system.Sequencing: this touches
internal/handlers/source_management.go,internal/server/routes.goandtemplates/source_logs.html, all of which #158 is changing. It gets dispatched once that squash-merges intonext, not before.Plan.
Route
GET /source/{sourceID}/logs/{eventID}/body, registered inside the existing/source/{sourceID}group so it inheritsRequireAuth,NoCacheand the rest.Authorization: the
id = ? AND user_id = ?lookup is currently copy-pasted per handler. I extract it once asownedWebhookand call it from bothHandleSourceLogsand the new handler, so the download cannot drift from the page it belongs to. Event lookup is then scopedid = ? AND webhook_id = ?, so an event id from another webhook is a miss, not an IDOR. Both misses arehttp.NotFound.Streaming:
modernc.org/sqlitereaches us throughdatabase/sql, which exposes no incremental BLOB handle, so there is no true row stream to take — scanning the column materialises it whole whatever wrapper is used. Instead I read the row in fixed 64 KiB chunks withsubstr(cast(body as blob), ?, ?)(the same byte-wise cast the#135projection relies on) and write each chunk straight to theResponseWriter, norenderTemplate. Peak resident body bytes is the chunk, not the payload. Deliberately no wrapping read transaction: these per-webhook databases are not in WAL mode, so holding a read lock across a slow client would block the receiver from writing new events. I will state the bound as what it is in the PR body rather than calling it streaming from the row.Headers:
application/octet-stream,Content-Disposition: attachment,Content-Lengthfrom alength(cast(body as blob))query, andX-Content-Type-Options: nosniffset on the response itself — the globalSecurityHeadersmiddleware does set it, but the route should not depend on a distant middleware for its own security control. The filename is built from the event id only afteruuid.Parseaccepts it, so nothing client-supplied reaches the header. The existing CSP does not help here (script-src 'self' 'unsafe-inline'would permit an inline script in a document served from our own origin); the disposition is the control.Template: the
#135truncation marker gains a download link, rendered only under{{if .BodyTruncated}}.Tests: owner fetches an over-cap body byte-identical; another user's event 404s; an event id belonging to a different webhook 404s; the disposition and content type are asserted; a
<script>body is served as an attachment and unaltered.Built as planned above; #167 carries the detail and the gate evidence. One correction to the plan, since it changes what a reader should believe about the route:
The
webhook_id = ?predicate I described as closing the IDOR is not what closes it. Events live in a per-webhook SQLite file, so a sibling webhook's event is not in the database being queried at all. Mutating both queries to drop that predicate leaves the cross-webhook test green. The predicate stays as a second guard and the code says which mechanism is load-bearing. The ownership mutation does fail as expected, serving the other user's payload with a 200.Verified with
make check(exit 0) anddocker build --no-cache-filter=lint --no-cache-filter=builder .(exit 0,0 issues.frommake lint, no cached test packages), both re-run after rebasing onto currentnext.