Conversation
Every user_agent column in the schema is VARCHAR(1024) -- audit.audit_log, auth.devices and auth.admin_sessions -- and nothing truncated anywhere. The header arrived verbatim from the request and went straight into the statement, so past 1024 characters PostgreSQL answered 22001. On the audit path that is the whole row. The insert is synchronous by default, every call site discards the error as best-effort so logging can never block authentication, and no metric counts it. So an unauthenticated caller could erase their own trail from an append-only log by setting a long header, and a credential-stuffing run would leave no login_failure rows behind it. Detection survives and caps the severity: audit.Logger runs its observers on the entry before the insert, so alerting and the anomaly detector still fire on an event whose row never lands. What is lost is the record afterwards. Clamped in the repository rather than at the handlers, for the reason actorColumns is there: the constraint belongs to the column and only this package knows the column, so every caller is covered rather than the ones somebody remembered. All four write sites take it. By runes, not bytes. PostgreSQL counts VARCHAR(n) in characters, so a byte slice would cut in the wrong place and would split a multi-byte rune, handing the driver invalid UTF-8 -- the same lost row with a more confusing reason. The test builds a 4200-byte, 2100-rune agent specifically to catch that. Verified against a real PostgreSQL: the raw column refuses 2000 characters with SQLSTATE 22001, the repository lands the row at 1024, and removing the clamp fails the test with the original error.
The clamp's fast path returns early when the byte length is already within the column, then falls through to a rune count. Nothing exercised the space between those two: a value over 1024 bytes but under 1024 characters, which the column accepts whole and a byte-length check alone would truncate. 600 three-byte runes is 1800 bytes and 600 characters, and the test asserts both of those before asserting the value comes back untouched -- so it cannot pass while silently testing something narrower.
Owner
Author
|
This PR sat for three weeks while main moved, and its head commit is already contained in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An unauthenticated caller could erase their own trail from an append-only log by setting a long header.
Every
user_agentcolumn in the schema isVARCHAR(1024)—audit.audit_log(001:159),auth.devices(001:84),auth.admin_sessions(001:260) — and nothing truncated anywhere. The header arrived verbatim from the request and went straight into the statement, so past 1024 characters PostgreSQL answered22001.On the audit path that is the whole row: the insert is synchronous by default, every call site discards the error as best-effort so logging can never block authentication, and no metric counts it. A credential-stuffing run would leave no
login_failurerows behind it.Detection survives, which caps the severity.
audit.Loggerruns its observers on the entry before the insert, so alerting and the anomaly detector still fire on an event whose row never lands. What is lost is the record afterwards — forensic, not detective.Clamped in the repository
Not at the handlers, for the reason
actorColumnsis there: the constraint belongs to the column, only this package knows the column, and a handler-level fix covers the call sites somebody remembered. All four write sites take it.By runes, not bytes. PostgreSQL counts
VARCHAR(n)in characters, so a byte slice would cut in the wrong place — and would split a multi-byte rune, handing the driver invalid UTF-8 and turning a value-too-long into an encoding error. Same lost row, more confusing reason.TestClampUserAgent_DoesNotSplitARunebuilds a 4200-byte / 2100-rune agent specifically to catch that, and asserts the result is still valid UTF-8, is exactly 1024 runes, and exceeds 1024 bytes — so the test cannot pass while silently testing the wrong thing.Verified against real PostgreSQL:
The raw column refuses 2000 characters; the repository lands the row at 1024. Removing the clamp fails the test with the original error and the sentence that matters:
go test -race ./internal/repository/postgres/,tests/spec+tests/compliancegreen,golangci-lint run ./...0 issues on a cleaned cache.