Repository navigation
feat(relay): log reads, logins and connections on access target - #8376
wpfleger96 wants to merge 14 commits into
Conversation
2287045 to
1fa935a
Compare
🔐 Codex Security Review
|
1a3e31d to
1ecd8ae
Compare
Signed-off-by: Will Pfleger <wpfleger@block.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up review via Wes’s account, integrating Rocket, Mantis and Groot review lanes.
The previous read-level and late-suppression findings have source fixes. One P3 follow-up remains inline: WARN-only error records lose the filter summary because its collection is still gated on INFO. This independently corroborates the existing security-bot comment.
Reviewed head b0a3747bf84b4fcb76e9068fcefcdbfe7f754c00 against base fcdabcf774203884c4f5b258aad000a740586eb6, focusing on changes since the previous reviewed head 98791374a445cc69adf74f103d77d786ee63e3e6 and merge integration. The public description and discussion contained no attached images or actionable disclosure found in this pass.
Source inspection only: no tests, builds, app launches or live requests were run. CI was not checked in this pass. Runtime cancellation, log filtering and deployed log collection remain unverified.
This reverts commit b0a3747. Signed-off-by: Will Pfleger <wpfleger@block.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>
…ogging * origin/main: perf(mobile): retain preference crypto across reconnects (#8442) fix(deps): bump webpki crates for RUSTSEC-2026-0381/0382/0383 (#8499) fix(mobile): simplify and refine the quick actions menu (#8491) fix(sdk): reject malformed mention pubkeys before signing p tags (#6372) fix(mobile): resolve DM display labels once per sort (#8488) Signed-off-by: Will Pfleger <wpfleger@block.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
No changes requested in the three-commit follow-up. The previous WebSocket WARN-summary finding is fixed through the real REQ/COUNT constructors. The atomic close marker accounts for late refusals without retaining the close record through detached reads; existing handler permits remain intact. Source assessment: minimalism, elegance and correctness each 9/10 for this follow-up.
Optional separate follow-up: the unchanged HTTP summary guards in AccessSlot::read and ProvenCaller::read_artifact still require INFO, so HTTP failures at WARN can omit read scope. This predates these fixes and is outside the focused follow-up; it is not a change request here.
Reviewed head cdfb12eb399b03be22af05ba324bcd4b5d3ce9a5 against base fcdabcf774203884c4f5b258aad000a740586eb6, focusing on changes since b0a3747bf84b4fcb76e9068fcefcdbfe7f754c00 and affected callers. The public description and discussion contained no attached images or actionable disclosure found in this pass.
Source inspection only: no tests, builds, app launches or live relay requests were run. CI was not checked. Runtime cancellation timing, logging throughput and deployed log collection remain unverified.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up review via Wes’s account.
No changes requested. Since the reviewed head cdfb12e, the feature change replaces prev < REFUSAL_LINES || prev >= CLOSED with the equivalent negated half-open range check. Boundary behavior and atomic ordering are preserved. The main merge has no conflict-resolution changes; the existing tracing-subscriber lockfile addition is unchanged. Minimalism, elegance and correctness: 9/10 each for this follow-up.
Reviewed head 93e3daf936243b36ed9b0ff7191ad5d65ad67bad against base a93f3282cc017e8decc8ea803d92967c4c3e36df. This uses the small unchanged-feature follow-up exception to team review; the previous feature review had independent team coverage. Public description and discussion inspected: no attached images or actionable disclosure found.
Source inspection only. No tests, builds, apps or live relay requests were run; CI was not checked. Runtime cancellation, logging throughput and deployed collection remain unverified.
Denied, failed and aborted HTTP requests log at WARN, but the read summary was only built at INFO, so under access=warn those lines lost their scope. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
|
🤖 Addressed in fe77157. Thanks for catching the HTTP side. |
Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
No changes requested in this two-commit follow-up. The HTTP read-summary guards now retain collected scope under WARN-only logging, including the artifact path. The shared WARN capture exercises the real summary producers and HTTP formatter; the artifact regression is included in the unit-test selection. Minimalism, elegance and correctness: 9/10 each for this follow-up.
Reviewed head 48e62d810eff2a2a1e332de2f8183ed78bbb69f7 against base a93f3282cc017e8decc8ea803d92967c4c3e36df, focusing on changes since reviewed head 93e3daf936243b36ed9b0ff7191ad5d65ad67bad and affected callers. Public description and discussion inspected: no attached images or actionable disclosure found.
Source inspection only: no tests, builds, apps or live requests were run. One CI snapshot showed 71 successful, 3 failed, 27 skipped and 2 running checks. The failures are Desktop Smoke E2E (10) and its two aggregate checks; their cause was not established in this review. Unchanged limitation: artifact cancellation during the query await can occur before its summary is recorded; this follow-up does not fix that ordering. Runtime cancellation, logging throughput and deployed collection remain unverified.
🤖 Adds an access log for relay reads, logins, connections and HTTP requests on the dedicated
buzz_relay::accesstarget. It writes to stdout, notaudit_log, so reads never take the audit advisory lock. The log is shaped for volume: one line per HTTP request, one line per WebSocket read, and one line per WebSocket connection.Lines per operation
http), except the skip list belowhttp,route="<unmatched>") at status 400 and above, otherwise noneclose, carrying the login)REQ/COUNTreq/count)auth)auth, at its final admission check)EVENTEvent ingested, not on the access target)HTTP requests
One middleware,
http_access_log, writes every HTTP line. It sits betweentrack_metricsandhttp_trace_layer, so rebuilt errors, body-limit refusals and localhost refusals all reach it. It writes the line when the response is ready, which is not proof the body was delivered. A request dropped before then logsrequest=abortedat WARN with nostatus.route(the matched route template, never the raw path),http_method,peer_addrwhen the listener has one,statusandtime_to_response_ms.AccessSlot: the login (method,pubkey,pubkey_verified,community), the decision, the filter summary, and the route's own fields. The decision isoutcome(success/denied/failed/challenged) with a fixedreason, orundecidedwhen the route recorded none. A refusal is final: a later admission doesn't replace it, so a replayed NIP-98 proof that verifies and is then refused logsfailed/nip98_replay.outcomedescribes the login. What the route did is instatusand the route's fields: a rejected/eventswrite logsoutcome=successwithstatus=400,accepted=falseand itsreject_reason./queryand/count: the filter summary andresult_count./events:accepted,kind,event_id, and on a refused write the parsecategory/line/columnor a fixedreject_reason(such asinvalidorrestricted), never the ingest message.invite_id,expires_atandmax_uses. Invite claim:joined.feedback_idandattachment_sha256. WithBUZZ_ADMIN_AUTH=disabledthey logmethod=network_trustedwith no key, and Host or Origin refusals log their fixed reason./media/{sha256_ext}template./,/info,/.well-known/nostr.json,/health,/_liveness,/_readiness, and the SPA and static files the relay serves when enabled (web assets, invite landing pages, git web pages, the admin bundle) at any status. Successful WebSocket upgrades (101) log nohttpline. Refusals before the upgrade on/and the audio path log one, with the status actually sent.outcome=undecidedwith no key.X-Pubkeyrequests logmethod=x_pubkey,pubkey_verified=false.challenged, with its 401) are INFO. Refusals, statuses of 400 and above, and aborted requests are WARN. In Enforce mode the line records the remapped status (for example 403). Denial bodies and headers are unchanged.WebSocket reads
REQ/COUNTlines carrypubkey,conn_id,sub_id, the filter summary, the outcome (served/denied/aborted/errorplus a fixed reason), andevents_queued(REQ) orcount(a COUNT that counted). A read logs WARN only when it ends inerror.served,deniedandabortedare INFO, sinceabortedmeans the client closed the connection mid-read./queryand/count:kinds, channel ids, authors andpvalues each list the first 10 distinct values plus an exact*_count. A dimension that any filter leaves open logs*.since/until/limittake the widest bound and are absent when any filter leaves them open, so no field understates what the read covered.idsis a count only. NIP-50 search logssearch: true, never the text. Only well-formed channel ids and pubkeys are listed. An artifact read's summary uses only the standard dimensions of each raw filter, so artifact predicates and client-defined tags (such as#1) are never logged. The summary is skipped when the access target is off. Under=warn, every WARN line, HTTP or WebSocket, still carries it.auth_required, quota, concurrency, overload) or denied are logged. The rest are counted asreads_suppressedon the close line. A refusal that arrives after the close line has been written, from a read still running when the client left, logs its own line, so every refusal is either logged or counted, never both. Served reads are never capped.WebSocket logins
A NIP-42 login writes one
authline when it is decided. A successful login logs INFOoutcome=successat its commit point, decided under the connection's close-reason lock, so a disconnect racing the login (session expiry, admin disconnect, revocation, ban or roster removal) either wins, and the login logs no success, or loses and closes the connection right after the success line. A refused or failed login writes one WARNauthline withoutcome, a fixedreasonfrom the sharedAuthReasoncode set,pubkeyandpubkey_verified. Root WebSocketauthlines carrypeer_addr. AUTH frames after the login is decided are capped at 16 per connection, and the rest are counted asauth_suppressed. An attempt cut off by close or session expiry logsaborted. Audio logins log oneauthline at their final admission check.Connections
The close line is the connection's only line. It is owned by the upgraded socket, so it is written exactly once however the connection ends: a normal close, a refusal while checking the community is active (
refused=not_admitted), the connection limit (refused=connection_limit), or the handler being dropped. It carriesconn_id,community,peer_addr,duration_ms, the login,events_queued/bytes_queued(frames the writer accepted and flushed, not client delivery), and the two suppression counters.Error text
DbError::kind()(SQLSTATE or variant), including the command-executor event insert. Workflow database and webhook errors log as their codes, since webhook URLs can be secrets.Operator notes
.env.exampledocuments the target, itseventvalues (http,auth,req,count,close), the skip list, the levels, and how to turn it down:buzz_relay::access=warnkeeps refusals, errors and aborted HTTP requests, and of WebSocket reads only those that end inerror, and=offdrops everything. It also lists the removed lines:NIP-42 auth successful/failed,WebSocket connection established/closed,event=connect,HTTP bridge request,Event ingested via pipeline,relay invite minted,relay member added via invite,admin feedback attachment read, andaddr(nowpeer_addr). JSON lines no longer repeat the current span asspan, sincespansalready ends with it. A line logged under an explicitparent:span loses that parent's fields. The default OTel export filter setsbuzz_relay::access=off, so access lines aren't exported a second time as span events.peer_addris the TCP peer.BUZZ_CLIENT_IP_HEADERoptionally names a request header (for examplecf-connecting-ipbehind Cloudflare orx-real-ipbehind nginx, matched case-insensitively). When set,http,auth(root and audio),closeand connection-limit lines also logclient_ip: the rightmost comma-separated entry of the header's last field, only if it parses as an IP address. It never replacespeer_addr, and it is only trustworthy when every request arrives through a proxy that overwrites the header.