Repository navigation
fix(gdpr): include system_logs/api_metrics/error_logs in DSAR export (#545) - #1880
Conversation
…545) DSAR export via collectAllUserData never touched the systemLogs, apiMetrics, and errorLogs monitoring tables, even though they retain a nullable, no-FK userId column right up until account deletion (deletion side was already covered by monitoring-purge.ts). Add three collectors mirroring collectUserActivity's pattern, wire them into AllUserData, and thread the new fields through both export-format.ts serializers (buildNativeExportFiles and toPortableExport enumerate AllUserData fields explicitly rather than deriving from Object.keys, so both had to be updated or the new data would be collected but silently dropped from the actual downloadable export). Redaction judgment call: each new *Export interface keeps only the what/when/where fields (timestamp, level/name, message, category, endpoint, method, duration/status, file/line/column, resolved) and excludes raw stack traces, IP addresses, user agents, session/request correlation IDs, opaque metadata blobs, and internal admin references (errorLogs.resolvedBy) — internal diagnostic/security telemetry that isn't data about the subject's own activity. Updated docs/security/gdpr-export-format.md to document the three new native files and portable additionalProperty entries. Refs #545
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR extends the GDPR user data export system to include three new monitoring categories: system logs, API metrics, and error logs. New collector functions and export interfaces are added to gdpr-export.ts, wired into native/portable export formats, surfaced in the account export API route, and documented, with corresponding test coverage updated throughout. ChangesMonitoring data in GDPR export
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExportRoute
participant collectAllUserData
participant Database
participant ExportFormat
Client->>ExportRoute: GET /api/account/export
ExportRoute->>collectAllUserData: fetch user data(userId)
collectAllUserData->>Database: query systemLogs, apiMetrics, errorLogs, others
Database-->>collectAllUserData: rows
collectAllUserData-->>ExportRoute: AllUserData
ExportRoute->>ExportFormat: buildNativeExportFiles(AllUserData)
ExportFormat-->>ExportRoute: system-logs.json, api-metrics.json, error-logs.json, others
ExportRoute-->>Client: ZIP archive
Related Issues: None specified. Related PRs: None specified. Suggested labels: compliance, gdpr, monitoring Suggested reviewers: None specified. 🐰 A hop through logs both new and old, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
mockUserData was missing systemLogs/apiMetrics/errorLogs, which are now required AllUserData fields. Since export-format.ts's buildNativeExportFiles/toPortableExport unconditionally read data.systemLogs.length etc., calling them with the stale fixture threw a runtime TypeError, failing CI (ci / Unit Tests). Also bump the two hardcoded archive.append() call-count assertions by 3 for the new system-logs.json/api-metrics.json/error-logs.json files, and assert their presence alongside the other category assertions. My earlier local verification of this file was invalid: I git-stash'd the source changes but never rebuilt packages/lib's dist output, so apps/web (which imports the built package) was still running against the new compiled code with the old test fixture, making the "before" comparison meaningless. Verified this time via the actual CI command (bun run test:coverage) after a full rebuild. Refs #545
…note - Add a CHANGELOG.md Fixed entry for the expanded DSAR export (repo CLAUDE.md requires changelog updates for user-visible changes). - Add null-value coverage for the nullable fields on the 3 new collectors/interfaces (systemLogs.category/endpoint/method/duration, apiMetrics.requestSize/responseSize, errorLogs.resolved/endpoint/ file/line/column), matching the existing null-path testing bar used elsewhere in this file (collectUserActivity's metadata: null, etc.). Also swap export-format.test.ts's lossless-fixture entries for these 3 categories to exercise null fields, matching how the other categories in that same fixture already do. - Add a one-line comment above collectAllUserData's Promise.all noting the array/destructuring order must stay in lockstep — this tuple grew from 10 to 13 positions in this PR with no compiler-checkable safety net against a future reorder mismatch. Refs #545
1 participant
Summary
system_logs,api_metrics, anderror_logs, even though those tables retain a nullable, no-FKuser_idcolumn right up until account deletion.activity/security_audit/system_logs/api_metrics/error_logs/ai_usage, plus data-residency/egress policy). This PR is a scoped slice covering only the confirmed DSAR export gap for the three named monitoring tables; the issue is intentionally left open pending the rest of that broader work, which is out of scope here. See the issue comment for two follow-ups I found but deliberately did not fold into this PR:security_audit_logis also still missing from the export, and the unbounded (no-LIMIT) query pattern this PR extends tosystem_logs/api_metricscould use pagination at scale (precedent: PR fix(gdpr): complete GDPR Art. 15 data export coverage #1084).packages/lib/src/logging/monitoring-purge.tsalready purges all three tables on erasure) — the gap was entirely on the export side.collectUserSystemLogs,collectUserApiMetrics,collectUserErrorLogstopackages/lib/src/compliance/export/gdpr-export.ts, mirroringcollectUserActivity's existing pattern, and wired them intoAllUserData/collectAllUserData.packages/lib/src/compliance/export/export-format.ts— itsbuildNativeExportFilesandtoPortableExportfunctions manually enumerate everyAllUserDatafield (not derived fromObject.keys), so without this change the new data would be collected from the DB but silently dropped from the actual ZIP/portable export a user downloads.docs/security/gdpr-export-format.mdandCHANGELOG.md(per repo CLAUDE.md's changelog rule for user-visible changes) to document the three new native files and portableadditionalPropertyentries.Redaction judgment call
Each new table carries internal diagnostic/security telemetry alongside the fields relevant to a data subject:
ip,userAgent,errorStack/stack,sessionId,requestId, opaquemetadatajsonb, and (forerror_logs)resolvedBy(an internal admin's user id, not the subject's).I chose to export only the fields describing what happened, when, and where:
UserSystemLogExport:id, timestamp, level, message, category, endpoint, method, durationUserApiMetricExport:id, timestamp, endpoint, method, statusCode, duration, requestSize, responseSizeUserErrorLogExport:id, timestamp, name, message, endpoint, method, file, line, column, resolvedExcluded: raw stack traces, IP addresses, user agents, session/request correlation IDs, opaque metadata blobs, and internal admin references. This is a deliberate exclusion, not an oversight. Happy to revisit if legal/compliance wants stack traces or IPs included for completeness.
Test plan
bun test(vitest) forpackages/lib/src/compliance/export/— 48/48 new+existing tests pass (35 ingdpr-export.test.ts, 13 inexport-format.test.ts, including null-path coverage for every nullable field on the 3 new collectors), 217+/217+ acrosspackages/lib/src/compliancepassbunx tsc --noEmitclean forpackages/lib,apps/web,apps/adminapps/web/.../account/export/route.tsandapps/admin/.../export/route.tsneed no changes (thin delegator + object-spread, respectively) — verified by reading bothapps/web/src/app/api/account/export/__tests__/route.test.ts— itsmockUserDatafixture was missing the 3 new requiredAllUserDatafields, which madebuildNativeExportFiles/toPortableExportthrow a runtimeTypeErroronce called with a stale fixture; fixed the fixture, updated two hardcodedarchive.append()call-count assertions (+3), and added presence assertions for the 3 new files. All 17 tests in this file now pass, verified via the actual CI command (bun run test:coverage) after a full monorepo rebuild.collectAllUserData(added a one-line invariant comment) — see commit history for details.Refs #545
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com