Access log and JSON logging mode for Kibana

[krokicki]

x2s3 had no access log of its own, so request timings only ever existed in uvicorn's text output where they could not be aggregated. This adds an access log and an optional JSON output format, so latency can be charted in Kibana.

Setting log_format: json (or X2S3_LOG_FORMAT=json) writes one Elastic Common Schema object per line to stdout, which a shipper parses straight off the container's logs. Text remains the default and stays human-readable; nothing changes unless you turn this on.

What's in it

AccessLogMiddleware is pure ASGI rather than BaseHTTPMiddleware, because x2s3 mostly streams large objects: the app coroutine only returns once the last body chunk is sent, so event.duration is real transfer time rather than time-to-first-byte, and the response bytes can be counted on the way past.

Each access line carries event.duration (nanoseconds, the ECS unit), http.request.method, http.response.status_code, http.response.body.bytes, url.path/url.query/url.domain, client.ip, user_agent.original, trace.id, and labels.target — the dimension worth slicing latency by. trace.id is the same value already returned in the x-amz-request-id header, and it is bound to every line logged while serving the request, so a client can quote an id and you get the whole request.

ECS fields are written as dotted keys rather than nested dicts; Elasticsearch expands them on ingest, so the indexed document is the same one without any dict-building in the code.

Uvicorn's own access log is disabled wherever the middleware is installed, so requests are logged once. That means text mode changes slightly: the same one-line-per-request format, now with the duration, the byte count and the target. Doing it in code rather than with --no-access-log covers the pinned command line baked into the Docker image.

Also here

A pre-existing crash, found while reading the new JSON logs in production: html.escape() only accepts strings, so a None reaching an error template raised AttributeError and turned a handled upstream error into an unhandled 500 with a traceback. The trigger was a backend answering with an empty <Resource/>, which botocore parses to None. Guarded once in the templates where every caller routes through — the same crash was also reachable from list_objects_v2 forwarding its optional prefix as the key, and from HTTPException.detail, which is not required to be a string. The Resource fallback in handle_s3_exception is restored too: dict.get's default is skipped when the key is present but null, which is exactly this case.

Testing

tests/test_log.py covers both formats: JSON mode emits one parseable ECS object per request with a numeric duration, error statuses log at warning, and text mode stays human-readable. tests/test_xss_prevention.py gains six tests for the null-value crash, including the reported traceback end to end through handle_s3_exception. 91 pass.

Also verified under real uvicorn in both modes, confirming the access log is not duplicated and that uvicorn's own lines are routed through the JSON sink.

Docs

docs/StructuredLogging.md covers the field list, a Filebeat input snippet, and the Kibana side (set event.duration's field format to Duration/nanoseconds and a percentile agg reads as p95/p99 directly).

Claude Code Reviews

This PR also deletes the failed security review CI that was mistakenly merged onto main. We are experimenting with Claude Code review on this PR.

@StephanPreibisch @neomorphic

[krokicki]

@claude Run a security review on this PR

[claude]

Code review

Two issues. Checked for bugs and CLAUDE.md compliance (no CLAUDE.md files in this repo).

1. target skips _log_safe() in the access log — commented inline at x2s3/log.py:185. It is the only client-controlled field on the line that is not escaped, and get_target() stores it in scope state before the target name is validated, so it reaches the default text-mode log unfiltered.

2. Merging this PR deletes .github/workflows/security.yml from main.

The diff removes the 22-line Security Review workflow (anthropics/claude-code-security-review@main, on: pull_request, comment-pr: true). Nothing in the description mentions it, and it has no connection to access logging.

It looks inadvertent rather than intended. The branch history shows the file was created and then removed here while experimenting with review automation:

• 3493ba4 "added automated security review" — creates security.yml
• 7499f47 "try sonnet for security review"
• 221d2e1 "changed security review to code review" — deletes security.yml, adds review.yml
• f4227e3 "remove code review action" — removes review.yml

Meanwhile the equivalent security.yml landed on main independently via #32, and it is present on the current base commit (fa69eac). Because this branch carries an explicit deletion of that path, the merge resolves delete-wins and main loses its copy. claude-code-review.yml, claude.yml and python-ci.yml survive; what is lost is specifically the dedicated security-review job on every PR.

If keeping it was the intent, restoring the file on this branch before merge is enough.