fix(membership): log audit record failures instead of dropping them#1770
fix(membership): log audit record failures instead of dropping them#1770AmanGIT07 wants to merge 1 commit into
Conversation
Audit record writes in the membership audit helpers discarded their error. Route them through a helper that logs the full record when the write fails. The member operation still succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMembership audit resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d26df683-675f-40e6-b51d-a492932219c0
📒 Files selected for processing (2)
core/membership/service.gocore/membership/service_test.go
| args := []any{ | ||
| "error", err, | ||
| "event", record.Event, | ||
| "org_id", record.OrgID, | ||
| "resource_id", record.Resource.ID, | ||
| "resource_type", record.Resource.Type, | ||
| "resource_name", record.Resource.Name, | ||
| "occurred_at", record.OccurredAt, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant service code and nearby helpers.
sed -n '760,860p' core/membership/service.go
printf '\n--- search for actor enrichment and audit record creation ---\n'
rg -n "enrichActorFromContext|Actor|WarnContext|createAuditRecord|auditRecordRepository.Create|auditOrgMemberAdded|metadata|email" core/membership -S
printf '\n--- search for logger middleware or context enrichment ---\n'
rg -n "actor.*context|context.*actor|middleware.*actor|With.*Actor|enrich.*Actor|logger.*actor" -S .Repository: raystack/frontier
Length of output: 10010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,110p' internal/store/postgres/audit_record_repository.go
printf '\n---\n'
sed -n '190,260p' internal/store/postgres/audit_record.go
printf '\n---\n'
sed -n '220,280p' core/auditrecord/service.go
printf '\n---\n'
sed -n '1,120p' pkg/server/connect_interceptors/authentication.go
printf '\n---\n'
sed -n '1,120p' core/audit/context.goRepository: raystack/frontier
Length of output: 10862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the audit logger implementation to see whether WarnContext adds actor data automatically.
sed -n '1,240p' core/audit/logger.go
printf '\n---\n'
rg -n "type Logger|func \(.*WarnContext|WithAttrs|extractActorFromContext|AuditActorContextKey|AuditRecordActorContextKey" core/audit internal/store/postgres pkg/server -SRepository: raystack/frontier
Length of output: 2831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' pkg/server/connect_interceptors/logger.go
printf '\n---\n'
rg -n "WarnContext|WithContext|context.*logger|actor.*log|AuditActorContextKey|AuditRecordActorContextKey" pkg/server/connect_interceptors core -SRepository: raystack/frontier
Length of output: 4954
Include the actor ID in the fallback log
WarnContext only carries request-scoped attrs here, and this path still logs an empty actor because the repository fills it in later. Add the actor ID from context so a failed audit write can be recreated fully.
Source: Learnings
| if record.Target != nil { | ||
| args = append(args, | ||
| "target_id", record.Target.ID, | ||
| "target_type", record.Target.Type, | ||
| "target_name", record.Target.Name, | ||
| "target_metadata", record.Target.Metadata, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact PII from the fallback audit logs.
Logging record.Target.Name and record.Target.Metadata directly writes Personally Identifiable Information (PII) — such as user names and email addresses (which are added to the metadata in methods like auditOrgMemberAdded) — to the standard application logs. While your secure audit database is designed to store this, standard log aggregators often lack the same strict data retention and compliance controls, risking GDPR/CCPA violations.
Consider redacting or hashing PII fields from the log arguments before logging.
Coverage Report for CI Build 29572653683Coverage increased (+0.03%) to 46.139%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
2 participants
Summary
Audit record writes in the membership audit helpers discarded their error. They now go through a helper that logs the failure. The member operation still succeeds.
Changes
core/membership: route the seven audit record writes through a newcreateAuditRecordhelperAddOrganizationMemberTechnical Details
No caller-visible behavior change: the audit helpers stay fire-and-forget.
Test Plan
go test ./core/membership/passes