Basekick-Labs / Basekick-Labs/arc

audit: middleware stores zero-copy Fiber strings in events that a background writer serializes later

Open
#837 3 comments 0 reactions 0 assignees View on GitHub
bug good first issue
Dominant language
Go
Stars
677
Forks
53
Avg merge
9h 14m
Merged PRs (30d)
164

Description

Found while reviewing #830.

## Problem

`internal/audit/middleware.go` builds an `AuditEvent` from `c.Path()`, `c.IP()`, `c.Get("User-Agent")`, `c.Get("x-arc-database")`, `c.Params(...)` and `c.Query("db")`, then hands the event to `Logger.LogEvent`, which only enqueues the pointer on a channel. The batch writer (`writerLoop`) serializes the fields up to a second later.

Arc runs Fiber with `Immutable=false` (see the note in `internal/api/msgpack.go`), so every one of those accessors returns a string that aliases a pooled buffer: `c.Query` is `app.getString(QueryArgs().Peek(key))`, `c.Path()` is `app.getString(c.pathBuffer)` on the pooled `Ctx`, and `c.Get` reads the request header buffer. Once the handler returns, the connection's `RequestCtx` and the `Ctx` go back to their pools and the next request overwrites those bytes. An audit row can therefore record a later request's path, database or user agent, or garbage. #830 adds `path` and `reason` from `c.Query` to the same event through `audit.DetailLocalsKey`, which the review asks to clone in the handler.

This is the same class as the retained `c.Get`/`c.Query` aliasing found in the edge-sync receive path (#776/#783 reviews).

## Fix shape

Copy every request-derived string when the event is built: `utils.CopyString` from `github.com/gofiber/fiber/v2/utils` on `path`, `c.IP()`, the user agent (after truncation), database, measurement, and any map supplied through the locals key (copy keys and values into a fresh map). `tokenInfo.Name` comes from the auth cache and needs no copy. Add a test that sends two different requests over one connection with `app.Test` disabled in favor of a real listener, or unit-test the copy by mutating the source buffer after the event is built and asserting the event is unchanged.

Small, self-contained, and a good first issue once the recipe above is followed exactly.

Contributor guide

Open the contributing guide

Research direction

Start in internal/audit/middleware.go and trace how request-derived values enter AuditEvent before Logger.LogEvent queues it; check the Immutable=false context in internal/api/msgpack.go and the locals supplied through audit.DetailLocalsKey. Copy the listed strings and map data when building the event, then verify with either a real-listener two-request test or a source-buffer mutation test that serialized events remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, observability
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
82/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.