fix(admin): attribute email-plane audit rows to an id the column can hold - #92
Open
42-v wants to merge 1 commit into
Open
fix(admin): attribute email-plane audit rows to an id the column can hold#9242-v wants to merge 1 commit into
42-v wants to merge 1 commit into
Conversation
…hold Every write to the admin email plane produced no audit row at all. The four audit calls in email.go passed h.actor(r), which returns a username. audit.audit_log.user_id is UUID, and the bootstrap admin's username is literally "admin", so the insert failed 22P02. The admin gateway wires a zero flush interval, so the logger runs synchronously and the error came straight back -- where every call site discards it as best-effort under #nosec G104. The handler answered 200 and the row simply did not exist. Of the audit call sites in this package these four were the only ones not passing an id, which is why nothing else in the admin plane showed the symptom. It matters most here: email:write rewrites the body of password-reset and verification mail for every user of an app, which makes it the route in this API a phisher would want and the one whose trail was empty. actorID returns a.ID and is used at those four sites only. email.go's UpdatedBy and CreatedBy still take actor(): those are display VARCHAR columns that correctly want a username. The gate that should have caught this could not. Its actor was "adm-actor-1" -- not UUID-shaped either -- so it was comparing two strings the column cannot hold and a mock that accepts any string kept it green. It now uses a real UUID and asserts the shape, which is what makes it a statement about the column rather than about equality, and it covers all four email routes. Restoring the username fails it by name. Note this is degraded attribution rather than silent loss as of #75: actorColumns now rescues such a row into metadata as actor_user_id_raw. The column is still blank, so the admin audit view and CountByUser cannot find it by actor, which is the half this fixes.
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.
Every write to the admin email plane produced no audit row at all.
The four audit calls in
email.gopassedh.actor(r), which returns a username.audit.audit_log.user_idisUUID, and the bootstrap admin's username is literally"admin"— so the insert failed22P02. The admin gateway wires a zero flush interval, so the logger runs synchronously and the error came straight back, where every call site discards it as best-effort under#nosec G104. The handler answered 200 and the row simply did not exist.Of the audit call sites in this package these four were the only ones not passing an id, which is why nothing else in the admin plane showed the symptom. It matters most here:
email:writerewrites the body of password-reset and verification mail for every user of an app — the route in this API a phisher would want, and the one whose trail was empty.actorIDreturnsa.IDand is used at those four sites only.email.go'sUpdatedByandCreatedBystill takeactor(): those are displayVARCHARcolumns that correctly want a username.The gate that should have caught this could not
TestPrivilegedMutations_AuditAttributeTheActorusedwantActor = "adm-actor-1"— not UUID-shaped either. So it compared two strings the column cannot hold, and a mock that accepts any string kept it green.It now uses a real UUID and asserts the shape, which is what makes it a statement about the column rather than about equality, and it covers all four email routes. Restoring the username fails it by name:
Severity note
As of #75 this is degraded attribution rather than silent loss:
actorColumnsnow rescues such a row into metadata asactor_user_id_raw. The column is still blank, so the admin audit view andCountByUsercannot find it by actor — that is the half this fixes.Verified:
go build ./...,go vet,go test -race ./internal/adminapi/,golangci-lint run ./...0 issues on a cleaned cache, misspell clean.