Skip to content

fix: audit log deep redaction and status capture#1287

Open
SharifIbrahimDev wants to merge 1 commit into
LabsCrypt:mainfrom
SharifIbrahimDev:fix/audit-log-security
Open

fix: audit log deep redaction and status capture#1287
SharifIbrahimDev wants to merge 1 commit into
LabsCrypt:mainfrom
SharifIbrahimDev:fix/audit-log-security

Conversation

@SharifIbrahimDev

Copy link
Copy Markdown

Closes #1183

What does this PR do?

This PR addresses the security vulnerability where audit logs recorded the request but never the outcome, and where deep secrets could leak into the database because of shallow payload redaction.

Description

  • Recursive Redaction: Upgraded the sanitizePayload logic in auditLog.ts to recursively scan objects and arrays, safely redacting keys defined in sensitiveFields (like secret, token, signedTxXdr) at any nested depth.
  • Outcome Status Tracking: Created a database migration to add a nullable status (integer) column to the audit_logs table.
  • Asynchronous Response Logging: Hooked the audit log database insertion logic to the Express res.on('finish') event, accurately capturing the final res.statusCode representing success (200) or denial (403/400).
  • Test Coverage: Added missing test cases to auditLog.test.ts to simulate deep nested secrets and denied requests. Also updated eventIndexer test logic and assertions to account for the new column schema.

Note

A few unrelated endpoints in loanEndpoints.test.ts and background jobs were failing locally on the main branch before these changes, but auditLog.ts and eventIndexer.ts related tests passed with flying colors.

@ogazboiz ogazboiz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code is genuinely good: sanitizePayload now recurses into nested objects and arrays and redacts sensitive keys at any depth (verified nested signedTxXdr and details.token both redacted, publicInfo preserved), and the status capture on res.on("finish") with the new status column is correct. auditLog passes 4/4 and eventIndexer 16/16. but there's a migration hard-gate issue that would break existing databases:

  1. the new migration 1782699458929_add-status-to-audit-logs is BACK-DATED, its number is below the current highest (1795000000000), so it sorts before 13 already-applied migrations. i reproduced it on postgres: on an already-migrated db, migrate:up hard-errors with "Not run migration 1782699458929 is preceding already run migration 1783000000013_notifications-add-status" (node-pg-migrate checkOrder). that breaks every staging/prod db. rename it to a prefix above the highest, e.g. 1796000000000_add-status-to-audit-logs.js, keeping the same up/down body (the addColumn status integer nullable / dropColumn is correct and reversible).
  2. eventIndexer.ts: the ADMIN_CONFIG audit insert changed target from contract:${event.contractId} to null and set ip_address to "internal-indexer", which drops the contract id from admin-config audit rows. if that's intentional, say so; otherwise restore the contract target.
  3. drop the stray pr_body.md from the repo root.

also the branch is stale, so rebase on main after the above: git fetch origin && git rebase origin/main && git push --force-with-lease. rename the migration and sort out the eventIndexer target and this is basically there.

if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] Audit log records the request but never the outcome, and redaction is shallow so nested secrets are persisted

2 participants