fix(audit): keep the row when the actor id is not a UUID - #75
Merged
Conversation
42-v
enabled auto-merge (squash)
August 26, 2026 16:00
user_id and client_id are UUID in the schema and string in the model, and three call sites pass a value the caller chose: the submitted client_id on a failed client auth, and the asserted subject on /mint and on a service document. A non-UUID there does not produce a row with an odd value in it. pgx refuses the encode and there is no row at all -- and the caller discards the error, correctly, because auditing must never block authentication. So the events most worth having are the ones most likely to vanish. A credential spray sends client_id=admin, not a UUID, which is exactly the case the comment above auditClientAuthFailure says the audit exists to catch: the trail records only the attackers who already know a valid client UUID. The same mechanism drops every /mint event whose subject is not a UUID, and that audit row is the compensating control docs/security.md AR-16 names for a by-design subject-forgery oracle it otherwise accepts. It also drops service document events, which spec section 7.8 says are filed under user_id precisely so an Art. 15 export finds them. actorColumns blanks an id the column cannot hold and moves the claimed value into metadata, which is JSONB and takes any string. The row lands, and it still records who the caller said they were. This sits in the postgres repository, not in audit.Logger. I wrote it in the logger first and four packages went red, which was the answer: the constraint belongs to the column, only this package knows the column, and normalising in the logger changes the shape of every AuditEntry in the process to satisfy a rule that applies at one boundary. It would also have left every mock repository accepting what the real one rejects -- which is how this survived in the first place. Tested both ways. actorColumns is pure and covered without a container, including that it leaves a valid uuid alone, accepts an uppercase one, and never mutates the caller's map. The mechanism itself is asserted against a real PostgreSQL: the INSERT is driven straight at the column to prove it still refuses "admin", and then through the repository to prove the row now lands with the claimed value readable at metadata->>'actor_client_id_raw'. One existing test moved with it. TestAuditRepo_RefusedCommitLosesTheBatchLoudly used UserID "u-1"; with a non-uuid actor the entry now carries a metadata map, which that harness cannot encode, so the batch would fail before reaching the commit and the test would pass for the wrong reason. It uses a real uuid now, because it is a test about a refused COMMIT and nothing else.
The misspell linter runs with locale US (.golangci.yml says the tree is predominantly US spelling and that switching raises the finding count from 115 to 413), so behaviour and normalised are findings in Go source however normal they look in the prose around them. On audit-actor: also restored the doc comment above AuditRepo.Insert. Inserting the actorColumns helper above the function left the comment attached to the helper, and revive caught the exported method with no comment. Only the audit branch has that half.
The first version of this fix tested the caller's id against the canonical
8-4-4-4-12 form and blanked anything else into metadata. That is narrower than
the type it was guarding.
PostgreSQL 8.12 accepts upper case, braces, and hyphens omitted or added after
any group of four digits, and normalises all of them to the same sixteen bytes.
So the dashless rendering is a uuid the column already holds, indexes and
returns dashed -- and it is what Guid.ToString("N") produces, on the .NET
platform this release is for. mintSubjectRe admits it, servicedoc admits it, and
audit.audit_log.user_id is a bare nullable uuid with no foreign key, so those
rows are attributed today and are in the Art. 15 export today.
Blanking them would have moved the actor into metadata and detached the row from
its subject permanently, in an append-only table, while looking like a
correctness fix. Canonicalising instead accepts everything the column accepts and
writes the rendering the column returns; a value that is not a uuid at all is
still kept as the caller's claim in metadata, which is the point of the original
change.
Also corrects the mechanism the comment named. pgx does not refuse the encode:
chooseParameterFormatCode returns TextFormatCode for any string before the oid
is consulted, so the value goes out as text and the server's parser is the
acceptor. The integration test now shows that directly -- SQLSTATE 22P02 from
PostgreSQL 16 -- and adds the round trip that a unit test cannot make: a
dashless id is written, stored canonically, and found by CountByUser under the
canonical spelling.
Two defects in the table test went with it. It cited normalizeActors, which was
the approach this deliberately does not take and which exists nowhere in the
tree, and its "no dashes" case carried a stray space, so the one case named for
the regression tested a string with a space in it instead.
42-v
force-pushed
the
fix/audit-actor-id
branch
from
August 26, 2026 19:40
ed5f8bd to
91fdcdb
Compare
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.
Merging this publishes nothing. vault42 releases are cut by pushing the annotated tag.
The defect
user_idandclient_idareUUIDin the schema (001:156-157) andstringin the model, and three call sites pass a value the caller chose: the submittedclient_idon a failed client auth, and the asserted subject on/mintand on a service document.A non-UUID there does not produce a row with an odd value in it. pgx refuses the encode and there is no row at all — and the caller discards the error, correctly, because auditing must never block authentication.
So the events most worth having are the ones most likely to vanish:
client_id=admin, not a UUID. That is precisely the case the comment aboveauditClientAuthFailuresays the audit exists to catch. The trail records only the attackers who already knew a valid client UUID./mintevent with a non-UUID subject. That row is the compensating controldocs/security.mdAR-16 names for a by-design subject-forgery oracle it otherwise accepts.spec.md§7.8 says are filed underuser_idso an Art. 15 export finds them.The fix, and why it moved
actorColumnsblanks an id the column cannot hold and moves the claimed value intometadata, which is JSONB and takes any string. The row lands, and it still records who the caller said they were.It sits in the postgres repository, not in
audit.Logger. I wrote it in the logger first and four packages went red — that was the answer, not an inconvenience:AuditEntryin the process to satisfy a rule that applies at one boundary.Verification
Pure function, no container needed:
covering: a valid uuid left alone, an uppercase uuid accepted, a spray's
admin, a legacy mint subject, both-bad, no-dashes, one-char-short, nil metadata, and that the caller's map is never mutated (a batch retry must not accumulate keys).Mechanism against a real PostgreSQL — the INSERT driven straight at the column to prove it still refuses
"admin", then through the repository to prove the row now lands with the value readable atmetadata->>'actor_client_id_raw'.Honest caveat: container-backed tests are intermittent in my environment —
RequireContainerRuntimeprobes with a 4s timeout and a cold podman often misses it, so that test skipped on one run and executed on another. CI is authoritative for it. Everything else here ran locally:internal/audit,internal/middleware,internal/handler,tests/specall ok,go vetandgofmtclean.One existing test moved with it
TestAuditRepo_RefusedCommitLosesTheBatchLoudlyusedUserID: "u-1". With a non-uuid actor the entry now carries a metadata map, which that harness cannot encode, so the batch would fail before reaching the commit and the test would pass for the wrong reason. It uses a real uuid now, because it is a test about a refused COMMIT and nothing else.Provenance
From the research + adversarial-hunt workflow, where it was named "the one thing to ship". Its argument is worth repeating: the product's differentiator is that its evidence is falsifiable — 456 requirements, a named test per Met row, CI failing when a Met row names a test that does not exist. This was a control that is documented, unit-tested, believed by the register, and silently absent at runtime for exactly the inputs an attacker sends.