fix(backend): fix data inconsistency issue in Audit Logger - #1410
Merged
emdevelopa merged 2 commits intoAug 28, 2026
Merged
Conversation
stableStringify() (used by sanitizeAuditValue, hashAuditPayload, and
signAuditPayload in audit-security.js) fell through to the generic
object-serialization branch for any Date value. Object.entries() on a
Date returns [] (Date has no own enumerable properties), so every Date
silently serialized to "{}" — discarding the actual timestamp entirely.
This is reachable anywhere a Date can end up as an audit field value,
e.g. auditService.logEvent's old_value/new_value for a profile change
to a timestamp field: the audit trail would record a meaningless "{}"
for both the old and new value instead of what actually changed, and
two audit events with genuinely different Date values would hash and
sign identically.
Fix: special-case `instanceof Date` before the generic object branch,
serializing via toISOString() the same way JSON.stringify would for a
top-level Date.
Closes emdevelopa#1331
Covers both symptoms of the bug: sanitizeAuditValue(date) now returns
the actual ISO timestamp instead of "{}", and hashAuditPayload produces
different hashes for payloads that differ only by Date value.
Ran locally: npx vitest run src/lib/audit-security.test.js — 21/21 passing.
|
@posimideveloper is attempting to deploy a commit to the Emmanuel's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@posimideveloper Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Closes #1334
Closes #1331
Closes #1329
Closes #1328
#1331 — done, closes it
Found a real Date-serialization bug in
backend/src/lib/audit-security.js'sstableStringify(), which backssanitizeAuditValue,hashAuditPayload, andsignAuditPayload.stableStringifyfalls through to a genericObject.entries(value)branch for any non-array object.Object.entries(new Date(...))returns[]—Datehas no own enumerable properties — so everyDatevalue silently serialized to"{}", discarding the actual timestamp. This is directly reachable throughauditService.logEvent'sold_value/new_valuefields for a profile change to a timestamp field: the audit trail would record a meaningless"{}"for both the old and new value instead of what actually changed, and two audit events with genuinely differentDatevalues would hash and sign identically — a real data-inconsistency bug in the audit trail's integrity guarantees.Fix: special-case
instanceof Datebefore the generic object branch, serializing via.toISOString()(matching whatJSON.stringifydoes for a top-levelDate).Added two regression tests to
audit-security.test.js: one assertingsanitizeAuditValue(date)now returns the real ISO timestamp instead of"{}", one assertinghashAuditPayloadproduces different hashes for payloads differing only byDatevalue. Ran locally:npx vitest run src/lib/audit-security.test.js— 21/21 passing.Note:
auditService.logEvent(the function whoseold_value/new_valuewould trigger this) currently has no live callers insrc/outside its own test — butsanitizeAuditValue/hashAuditPayload/signAuditPayloadare general-purpose exported utilities with this bug regardless, andlib/audit.js'slogLoginAttemptuses the samestableStringifymachinery, so the fix isn't dead-code-only.#1334, #1329, #1328 — not started
Both the "Ledger Monitor" (
ledger-monitor-security.js,horizon-poller.js) and "Audit Logger" (audit.js,auditService.js,audit-writer.js,audit-circuit-breaker.js,routes/audit.js) modules have already been through many rounds of security hardening, auditing, and load testing in this repo's history (git logshows repeated "security audit", "harden", "error recovery", "SQL optimization" commits referencing issues up to #1067, #911, #902, #772, #771, #770, #769, #768). I read through all of the above files looking for a genuine, currently-existing gap matching "security vulnerability" (#1334, #1329) or "null pointer exception" (#1328) and did not find one — validation, sanitization, rate limiting, circuit breaking, signature verification, and constant-time comparison are all already in place and look sound. Rather than fabricate a change to force a "fix," I'm leaving these three open and undone. If there's a specific vulnerability or NPE in mind that I missed, a file/line pointer would help narrow it down.No
Closesline for #1334, #1329, or #1328; they should stay open.Test plan
npx vitest run src/lib/audit-security.test.js— 21 passed, 21 total (includes 2 new regression tests)