feature: add GDPR compliance #9865
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds a GDPR-focused audit logging subsystem: logger, adapter, controller, option definitions, and integrations into auth, query, write, push, and schema flows, plus tests and a GDPR guide. Logs are structured JSON, daily-rotated, masked, and configurable via new auditLog options. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as Routers (Users/Push/Schemas)
participant Core as Core Logic
participant ALC as AuditLogController
participant ALA as AuditLogAdapter
participant AL as AuditLogger
participant FS as File System
rect rgb(245,248,252)
note over Router,Core: Normal request handling
Client->>Router: HTTP request
Router->>Core: Perform action
Core-->>Router: Result / Error
end
alt Audit logging enabled
Router->>ALC: log...({ auth, req, ... })
ALC->>ALA: event (masked, with user/IP)
ALA->>AL: logAuditEvent(event)
AL->>FS: append JSON line (rotated file)
else Disabled or no adapter
Router-->>Router: Skip logging
end
Router-->>Client: HTTP response
sequenceDiagram
autonumber
participant Client
participant RQ as RestQuery
participant DB as DB
participant ALC as AuditLogController
participant AL as AuditLogger
Client->>RQ: find()
RQ->>DB: execute query
DB-->>RQ: results
alt results && logging enabled
RQ->>ALC: logDataView({ className, query, resultCount, objectIds<=100 })
ALC->>AL: write event
else skip
RQ-->>RQ: no-op
end
RQ-->>Client: results
sequenceDiagram
autonumber
participant Client
participant RW as RestWrite
participant DB as DB
participant ALC as AuditLogController
participant AL as AuditLogger
Client->>RW: create/update/delete
RW->>DB: write operation
DB-->>RW: result
par Create/Update
RW->>ALC: logDataCreate/update({ masked data/fields })
and ACL change
RW->>ALC: logACLModify({ oldACL, newACL })
end
ALC->>AL: write event(s)
RW-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
GDPR_COMPLIANCE_GUIDE.md(1 hunks)spec/AuditLogAdapter.spec.js(1 hunks)spec/AuditLogController.spec.js(1 hunks)spec/AuditLogSchemas.spec.js(1 hunks)spec/AuditLogging.e2e.spec.js(1 hunks)spec/Auth.spec.js(1 hunks)spec/ParseObject.spec.js(1 hunks)spec/ParseQuery.spec.js(1 hunks)src/Adapters/Logger/AuditLogAdapter.js(1 hunks)src/Adapters/Logger/AuditLogger.js(1 hunks)src/Controllers/AuditLogController.js(1 hunks)src/Controllers/index.js(5 hunks)src/Options/Definitions.js(2 hunks)src/Options/index.js(2 hunks)src/RestQuery.js(2 hunks)src/RestWrite.js(2 hunks)src/Routers/PushRouter.js(2 hunks)src/Routers/SchemasRouter.js(3 hunks)src/Routers/UsersRouter.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
src/Controllers/index.js (3)
src/Adapters/Logger/AuditLogger.js (1)
options(81-86)src/Controllers/AuditLogController.js (1)
AuditLogController(8-320)src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)
src/Adapters/Logger/AuditLogAdapter.js (3)
src/Adapters/Logger/LoggerAdapter.js (1)
LoggerAdapter(9-18)src/Adapters/Logger/AuditLogger.js (4)
options(81-86)configureAuditLogger(58-89)isAuditLogEnabled(139-141)logAuditEvent(105-133)src/Controllers/index.js (11)
options(62-71)options(85-85)options(104-112)options(125-125)options(133-133)options(153-153)options(163-163)options(164-164)options(184-184)options(196-196)options(234-234)
spec/ParseObject.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)path(42-42)reconfigureServer(171-205)Parse(4-4)
spec/AuditLogController.spec.js (2)
src/Controllers/AuditLogController.js (1)
AuditLogController(8-320)src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)
spec/AuditLogging.e2e.spec.js (3)
spec/AuditLogAdapter.spec.js (4)
fs(5-5)require(4-4)path(6-6)testLogFolder(9-9)spec/helper.js (1)
reconfigureServer(171-205)spec/ParseQuery.spec.js (8)
Parse(7-7)i(374-374)i(599-599)i(2398-2398)i(2658-2658)i(3067-3067)i(3109-3109)request(8-8)
spec/AuditLogSchemas.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
src/Controllers/AuditLogController.js (3)
spec/AuditLogController.spec.js (2)
AuditLogController(3-3)AuditLogAdapter(4-4)src/Controllers/index.js (11)
options(62-71)options(85-85)options(104-112)options(125-125)options(133-133)options(153-153)options(163-163)options(164-164)options(184-184)options(196-196)options(234-234)src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)
spec/Auth.spec.js (1)
spec/helper.js (5)
require(6-6)require(8-8)require(52-52)reconfigureServer(171-205)Parse(4-4)
src/Routers/PushRouter.js (2)
src/PromiseRouter.js (1)
body(141-141)src/Controllers/index.js (1)
pushController(213-213)
src/Adapters/Logger/AuditLogger.js (1)
src/Controllers/index.js (11)
options(62-71)options(85-85)options(104-112)options(125-125)options(133-133)options(153-153)options(163-163)options(164-164)options(184-184)options(196-196)options(234-234)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
src/Routers/UsersRouter.js (1)
src/RestWrite.js (9)
user(330-330)user(901-901)user(1514-1514)user(1787-1787)userId(537-537)userId(547-547)RestWrite(988-995)RestWrite(1153-1159)sessionData(1010-1019)
spec/ParseQuery.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)reconfigureServer(171-205)Parse(4-4)i(324-324)
spec/AuditLogAdapter.spec.js (2)
src/Adapters/Logger/AuditLogAdapter.js (1)
AuditLogAdapter(9-293)src/Adapters/Logger/AuditLogger.js (1)
logAuditEvent(105-133)
🪛 Biome (2.1.2)
src/Controllers/index.js
[error] 84-84: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 84-84: return types can only be used in TypeScript files
(parse)
src/Options/index.js
[error] 217-217: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 545-545: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 545-545: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
[error] 548-548: Expected a type but instead found '?'.
Expected a type here.
(parse)
[error] 548-548: Expected a property, or a signature but instead found ';'.
Expected a property, or a signature here.
(parse)
[error] 553-553: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 558-558: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🪛 GitHub Check: Lint
src/RestQuery.js
[warning] 981-981:
Unexpected console statement
src/RestWrite.js
[warning] 1859-1859:
Unexpected console statement
spec/AuditLogging.e2e.spec.js
[failure] 442-442:
'request' is not defined
spec/AuditLogSchemas.spec.js
[failure] 226-226:
'request' is not defined
[failure] 197-197:
'request' is not defined
[failure] 164-164:
'request' is not defined
[failure] 133-133:
'request' is not defined
[failure] 117-117:
'request' is not defined
[failure] 82-82:
'request' is not defined
[failure] 66-66:
'request' is not defined
[failure] 35-35:
'request' is not defined
spec/Auth.spec.js
[failure] 321-321:
Empty block statement
src/Adapters/Logger/AuditLogger.js
[warning] 77-77:
Unexpected console statement
[warning] 46-46:
Unexpected console statement
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (7)
src/RestWrite.js (2)
157-159: LGTM! Audit logging correctly placed after cleanUserAuthData.The audit logging is invoked at the right point in the pipeline—after
cleanUserAuthDataensures sensitive auth data is cleaned from the response, and before the final response assembly. This prevents logging of raw authentication tokens.
1823-1830: AuditLogController masks sensitive fields in create logs
Confirmed thatlogDataCreatecalls_maskSensitiveData(data), replacing any sensitive values with'***masked***'. No further action needed.src/Routers/SchemasRouter.js (3)
91-119: LGTM! Schema create audit logging is correctly implemented.The try/catch wrapper ensures both success and failure scenarios are logged before propagating errors. Logging happens after the operation completes, ensuring the audit trail reflects actual outcomes.
122-163: LGTM! Schema modify audit logging is correctly implemented.The conversion to async/await with try/catch provides clean error handling, and audit events are emitted for both success and failure paths.
166-210: LGTM! Schema delete audit logging is correctly implemented.The async/await refactor maintains the existing validation logic while adding comprehensive audit logging for both success and failure scenarios.
src/Routers/UsersRouter.js (2)
359-418: LGTM! Master key login audit logging is well-structured.The handleLogInAs refactor properly wraps the entire flow in try/catch, ensuring that both successful and failed master key login attempts are audited. The loginMethod is correctly set to 'masterkey' to distinguish this authentication path.
202-342: Audit logging covers all login failure paths. Errors thrown synchronously or via promise rejections in_authenticateUserFromRequestalways propagate intohandleLogIn’s catch block, ensuringauditLogController.logUserLoginrecords every failure.
| this.config.auditLogController.logDataView({ | ||
| auth: this.auth, | ||
| req: { config: this.config }, // Minimal req object since we don't have access to full request here | ||
| className: this.className, | ||
| query: this.restWhere, | ||
| resultCount: this.response.results.length, | ||
| objectIds: objectIds, | ||
| }); | ||
| } catch (error) { | ||
| // Don't fail the query if audit logging fails | ||
| console.error('Audit logging error:', error); | ||
| } |
There was a problem hiding this comment.
Use Parse logger instead of console.error.
console.error violates our lint rules and bypasses the configured logger. Please swap this for this.config.logger.error(...) (or the equivalent) so lint passes and we respect centralized logging.
Here’s a concrete fix:
- } catch (error) {
- // Don't fail the query if audit logging fails
- console.error('Audit logging error:', error);
- }
+ } catch (error) {
+ // Don't fail the query if audit logging fails
+ this.config.logger?.error?.('Audit logging error', { error });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.config.auditLogController.logDataView({ | |
| auth: this.auth, | |
| req: { config: this.config }, // Minimal req object since we don't have access to full request here | |
| className: this.className, | |
| query: this.restWhere, | |
| resultCount: this.response.results.length, | |
| objectIds: objectIds, | |
| }); | |
| } catch (error) { | |
| // Don't fail the query if audit logging fails | |
| console.error('Audit logging error:', error); | |
| } | |
| this.config.auditLogController.logDataView({ | |
| auth: this.auth, | |
| req: { config: this.config }, // Minimal req object since we don't have access to full request here | |
| className: this.className, | |
| query: this.restWhere, | |
| resultCount: this.response.results.length, | |
| objectIds: objectIds, | |
| }); | |
| } catch (error) { | |
| // Don't fail the query if audit logging fails | |
| this.config.logger?.error?.('Audit logging error', { error }); | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[warning] 981-981:
Unexpected console statement
🤖 Prompt for AI Agents
In src/RestQuery.js around lines 971 to 982, replace the console.error call in
the audit logging catch block with the Parse-configured logger: call
this.config.logger.error(...) (or this.config.logger.warn(...) if appropriate)
and pass a descriptive message plus the caught error object so the centralized
logger records the failure without throwing (e.g., "Audit logging error" and the
error). Keep the surrounding try/catch behavior so the query does not fail when
audit logging fails.
| } catch (error) { | ||
| // Don't fail the write if audit logging fails | ||
| console.error('Audit logging error:', error); | ||
| } |
There was a problem hiding this comment.
Replace console.error with logger.
Using console.error is flagged by the linter and inconsistent with the rest of the codebase. The file already imports logger at line 18.
Apply this diff:
} catch (error) {
// Don't fail the write if audit logging fails
- console.error('Audit logging error:', error);
+ logger.error('Audit logging error:', error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| // Don't fail the write if audit logging fails | |
| console.error('Audit logging error:', error); | |
| } | |
| } catch (error) { | |
| // Don't fail the write if audit logging fails | |
| logger.error('Audit logging error:', error); | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[warning] 1859-1859:
Unexpected console statement
🤖 Prompt for AI Agents
In src/RestWrite.js around lines 1857 to 1860, replace the console.error call
used for audit logging failures with the project's logger (imported at line 18);
call logger.error with the same descriptive message and include the error
details so the log remains informative and consistent with the codebase's
logging practice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/AuditLogging.e2e.spec.js (2)
53-53: Consider making the wait time configurable or event-driven.The 500ms hardcoded wait on line 53 (and similar waits throughout the suite) could cause flakiness on slower CI systems or pass prematurely on fast systems. Consider either using a polling approach to check for log completion or making the timeout configurable via environment variable.
Example polling approach:
// Helper function to wait for logs async function waitForLogs(folder, minLines = 1, timeout = 5000) { const start = Date.now(); while (Date.now() - start < timeout) { if (fs.existsSync(folder)) { const files = fs.readdirSync(folder); if (files.length > 0) { const content = fs.readFileSync(path.join(folder, files[0]), 'utf8'); const lines = content.split('\n').filter(l => l.trim()); if (lines.length >= minLines) return; } } await new Promise(resolve => setTimeout(resolve, 50)); } } // Usage: await waitForLogs(testLogFolder, 5);
388-426: Consider making performance threshold configurable.The 10-second threshold on line 415 provides a baseline performance check, but different CI environments may have vastly different performance characteristics. Consider making this threshold configurable via environment variable or adjusting it based on empirical CI data.
Example:
const perfThreshold = parseInt(process.env.AUDIT_LOG_PERF_THRESHOLD) || 10000; expect(duration).toBeLessThan(perfThreshold);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
spec/AuditLogController.spec.js(1 hunks)spec/AuditLogSchemas.spec.js(1 hunks)spec/AuditLogging.e2e.spec.js(1 hunks)spec/Auth.spec.js(2 hunks)spec/ParseObject.spec.js(1 hunks)src/Adapters/Logger/AuditLogger.js(1 hunks)src/Controllers/AuditLogController.js(1 hunks)src/RestQuery.js(2 hunks)src/RestWrite.js(2 hunks)src/Routers/UsersRouter.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Adapters/Logger/AuditLogger.js
- spec/AuditLogController.spec.js
- src/Controllers/AuditLogController.js
🧰 Additional context used
🧬 Code graph analysis (5)
src/Routers/UsersRouter.js (2)
src/RestWrite.js (16)
user(330-330)user(901-901)user(1514-1514)user(1787-1787)authData(453-453)Auth(8-8)Auth(575-578)validatedAuthData(560-563)res(624-628)RestWrite(988-995)RestWrite(1153-1159)expiresAt(1009-1009)sessionData(1010-1019)error(666-666)userId(537-537)userId(547-547)src/triggers.js (2)
maybeRunTrigger(849-952)error(644-644)
spec/AuditLogSchemas.spec.js (2)
src/RestWrite.js (4)
request(821-827)request(956-962)schema(346-346)error(666-666)spec/helper.js (1)
reconfigureServer(171-205)
spec/ParseObject.spec.js (1)
spec/helper.js (6)
require(6-6)require(8-8)require(52-52)path(42-42)reconfigureServer(171-205)Parse(4-4)
spec/Auth.spec.js (2)
src/RestWrite.js (3)
request(821-827)request(956-962)error(666-666)spec/helper.js (2)
reconfigureServer(171-205)Parse(4-4)
spec/AuditLogging.e2e.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (34)
spec/Auth.spec.js (6)
3-3: LGTM! Clean test suite setup.The request import and test suite initialization follow best practices with proper temp folder cleanup in beforeEach/afterEach hooks.
Also applies to: 260-276
277-306: LGTM! Comprehensive successful login test.The test properly creates a user, performs logout/login, waits for logs to flush, and validates all expected log markers including masking.
308-340: LGTM! Failed login test properly handles errors.The empty catch block from the previous review has been correctly addressed with explicit error assertions (lines 324-327). The test now validates both the expected authentication failure and the corresponding audit log entry.
342-380: LGTM! loginAs test validates master key flow.The test properly exercises the loginAs endpoint with master key authentication and verifies the corresponding audit log entries.
382-405: LGTM! IP address capture test.Clean test validating that client IP addresses are captured in authentication logs.
407-422: LGTM! Negative test for disabled logging.Important test confirming that audit logging is properly disabled when the auditLog configuration is not present.
spec/ParseObject.spec.js (7)
2176-2197: LGTM! Clean CRUD audit test suite setup.The test suite initialization follows the established pattern with proper folder cleanup and server reconfiguration.
2199-2222: LGTM! Object creation audit test.Properly validates that DATA_CREATE events are logged with the expected className and objectId.
2224-2256: LGTM! Object update audit test.The test properly isolates the update operation by clearing initial creation logs, then verifies DATA_UPDATE logging.
2258-2291: LGTM! Object deletion audit test.Correctly captures the objectId before deletion and verifies DATA_DELETE logging.
2293-2326: LGTM! ACL modification audit test.Properly tests that ACL changes are logged separately as ACL_MODIFY events.
2328-2371: LGTM! Masking and user context tests.These tests validate two critical GDPR requirements: sensitive field masking (password) and user context capture (userId in logs).
2373-2389: LGTM! Master key without user test uses correct pattern.The test correctly uses the per-operation
{ useMasterKey: true }option (line 2377) rather than the globalParse.Cloud.useMasterKey()toggle. This validates that internal master-key operations without a user context are properly excluded from audit logs.src/RestQuery.js (2)
305-307: LGTM! Clean integration of audit logging into query execution.The logAuditDataView call is properly inserted after handleAuthAdapters and before returning the response, ensuring audit logging doesn't interfere with the main query flow.
952-985: LGTM! logAuditDataView implementation with past issue resolved.The method properly:
- Guards against disabled/missing audit controller
- Skips master operations without user context (GDPR requirement)
- Limits logged objectIds to 100 (performance consideration)
- Uses
this.config.loggerController.error(line 981) instead ofconsole.erroras flagged in the previous review- Never fails the query on audit logging errors
src/RestWrite.js (2)
157-159: LGTM! Clean integration of audit logging into write execution.The logAuditDataWrite call is properly positioned after cleanUserAuthData and before returning the response, mirroring the pattern in RestQuery.
1801-1867: LGTM! logAuditDataWrite implementation with all past issues resolved.The method correctly:
- Skips only master operations without user context (line 1808), not maintenance mode - addressing previous consistency concern
- Detects ACL changes including added/removed cases using
?? nullfallback (lines 1821-1823) - addressing previous incomplete detection concern- Uses
this.config.loggerController.error(line 1863) instead ofconsole.error- addressing previous linting concern- Logs create events with full data, update events with changed fields only, and ACL modifications separately
- Never fails the write on audit logging errors
spec/AuditLogSchemas.spec.js (6)
1-26: LGTM! Imports and setup with past issue resolved.The request helper import (line 5) that was missing in the previous review has been properly added. The test suite setup follows the established pattern with proper temp folder cleanup.
28-57: LGTM! Schema creation audit test.Properly tests that schema creation via the API generates SCHEMA_MODIFY audit logs with the create operation marker.
59-108: LGTM! Schema update audit test.Correctly isolates the update operation and verifies SCHEMA_MODIFY logging with the update operation marker.
110-154: LGTM! Schema deletion audit test.Properly tests that schema deletion generates SCHEMA_MODIFY audit logs with the delete operation marker.
156-215: LGTM! Failed schema creation and user context tests.The failed creation test properly handles the expected error for invalid class names and verifies failure logging. The user context test confirms that schema operations include user information in audit logs.
217-246: LGTM! Schema field details test.Validates that schema modifications include field-level details in the audit logs, providing comprehensive audit trail for schema changes.
src/Routers/UsersRouter.js (4)
318-326: SessionToken masking implemented correctly.The past review concern about sessionToken leakage has been addressed. Line 320 now masks the sessionToken as
'***masked***'before passing it toauditLogController.logUserLogin, preventing credential exposure in audit logs.
329-341: LGTM! Failure audit logging is well-structured.The catch block correctly logs failed login attempts with error details, extracts username from available sources, determines loginMethod, and rethrows the error to preserve error semantics.
393-402: LGTM! Master-key login audit logging is correct.The audit log correctly masks sessionToken, sets loginMethod to 'masterkey', includes appropriate username fallbacks, and follows the same pattern as handleLogIn.
405-417: LGTM! Master-key login failure logging is consistent.The catch block appropriately logs failed loginAs attempts with userId extraction, error details, and correct loginMethod, while preserving error semantics by rethrowing.
spec/AuditLogging.e2e.spec.js (7)
3-5: Import issue resolved.The missing
requestimport flagged in the previous review has been addressed. Line 5 now correctly imports the request helper.
80-151: LGTM! File management tests are comprehensive.The test suite thoroughly validates log file naming patterns, automatic folder creation, and custom date patterns. The regex patterns correctly match the expected formats based on configuration.
153-203: LGTM! Configuration tests validate dynamic behavior.The tests properly verify that audit logging can be disabled and dynamically enabled at runtime, which is important for operational flexibility.
205-290: LGTM! Content validation is thorough and security-focused.The test suite validates required fields, proper JSON formatting, and critically verifies that sensitive data like passwords and session tokens are masked and never appear in plain text in audit logs.
292-330: LGTM! Concurrent operations test validates thread-safety.The test appropriately validates that the audit logger handles concurrent operations correctly, ensuring all events are captured without loss or corruption.
332-385: LGTM! Error handling tests ensure audit logging is non-blocking.The tests appropriately verify that audit logging failures don't prevent core operations from succeeding, and that failed operations are properly logged with error details. This is critical for GDPR compliance without compromising system reliability.
428-463: LGTM! Cross-feature test validates schema audit integration.The test appropriately validates that schema modifications are audited with correct event types and entities, demonstrating integration beyond user operations as required for comprehensive GDPR compliance.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
GDPR_COMPLIANCE_GUIDE.md (4)
26-26: Scope correction: not “database‑level” loggingAudit events are emitted at the Parse Server request/operation layer (RestQuery/RestWrite/routers), not at the database layer. Please adjust wording to avoid overpromising.
- - Automatic, transparent logging at the database level + - Automatic, transparent logging at the request/operation layer in Parse Server
468-476: IP capture in Cloud Code:request.ipmay be unavailable
request.ipis not guaranteed in Cloud Code. Prefer reading IP from the inbound request context (e.g., Expressreq.ip) exposed by Parse Server if supported, or from a trusted header behind your proxy (X‑Forwarded‑For), normalized server‑side. Otherwise omit IP.- acceptance.set('ipAddress', request.ip); + // If available in your Cloud Code runtime: + const ip = (request.headers && request.headers['x-forwarded-for']) + ? request.headers['x-forwarded-for'].split(',')[0].trim() + : undefined; + if (ip) acceptance.set('ipAddress', ip);Please confirm which fields Parse exposes on
requestin your deployment.
525-565: Breach detection example: hard‑coded log filename and dateUsing a fixed filename (
2025-10-01) will break with rotation. Parameterize the folder/pattern and iterate recent files.- const logFile = './audit-logs/parse-server-audit-2025-10-01.log'; + const path = require('path'); + const folder = process.env.AUDIT_LOG_FOLDER || './audit-logs'; + const files = fs.readdirSync(folder) + .filter(f => f.startsWith('parse-server-audit-') && f.endsWith('.log')) + .sort() // latest last if date in name + .slice(-3); // check last N files + for (const f of files) { + const fileStream = fs.createReadStream(path.join(folder, f)); + // ... process lines as below + }Also define
notifySecurityTeamor make the call path explicit.
639-655: Tamper‑evidence: prefer proven WORM/immutability over “blockchain”Recommend S3 Object Lock, Azure Immutable Blob, or append‑only/WORM filesystems with retention enforcement instead of suggesting blockchain.
- - Backup to immutable storage - - Consider blockchain for tamper-evidence + - Backup to immutable storage (e.g., S3 Object Lock, Azure Immutable Blob) + - Enforce WORM/retention policies for tamper‑evidence
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
GDPR_COMPLIANCE_GUIDE.md(1 hunks)
🔇 Additional comments (2)
GDPR_COMPLIANCE_GUIDE.md (2)
433-440: Replace in-app scheduling in docs with external schedulerUsing
node-schedulein multi-instance servers duplicates jobs. Invert control: remove theschedule.scheduleJobsnippet inGDPR_COMPLIANCE_GUIDE.mdand instead call the job endpoint from an external scheduler. For example:-// Run daily at 2 AM -schedule.scheduleJob('0 2 * * *', async () => { - await Parse.Cloud.startJob('enforceDataRetention'); -}); +// Use external scheduler (cron/K8s CronJob/GitHub Actions) to POST /jobs/enforceDataRetention +// Example (cron): +// 0 2 * * * curl -X POST "https://your-server/jobs/enforceDataRetention" \ +// -H "X-Parse-Master-Key: <masterKey>"
Parse.Cloud.startJobis supported by Parse Server and can be invoked via the REST endpoint when called externally.
39-49: Config snippet: align keys/defaults and add security caveat
- Use exact
auditLogconfig keys and defaults from src/Options/Definitions.js (auditLogFolder, datePattern, maxSize, maxFiles).- Add a note that audit logs may contain personal data and must be stored on restricted, immutable storage.
| - You get automatic compliance with GDPR Article 30 (Records of Processing Activities) | ||
| - You have an audit trail for Article 33 (Data Breach Notification) | ||
| - You have evidence for Article 32 (Security of Processing) | ||
|
|
There was a problem hiding this comment.
Avoid compliance guarantees; soften language
The guide currently implies “automatic compliance” with Articles 30/33/32. The feature “supports” compliance but does not guarantee it. Please rephrase.
- You get automatic compliance with GDPR Article 30 (Records of Processing Activities)
- You have an audit trail for Article 33 (Data Breach Notification)
- You have evidence for Article 32 (Security of Processing)
+ Helps you maintain records relevant to Article 30 (Records of Processing Activities)
+ Provides audit trails that support Article 33 (Breach Notification)
+ Provides evidence that can support Article 32 (Security of Processing)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - You get automatic compliance with GDPR Article 30 (Records of Processing Activities) | |
| - You have an audit trail for Article 33 (Data Breach Notification) | |
| - You have evidence for Article 32 (Security of Processing) | |
| - Helps you maintain records relevant to Article 30 (Records of Processing Activities) | |
| - Provides audit trails that support Article 33 (Breach Notification) | |
| - Provides evidence that can support Article 32 (Security of Processing) |
🤖 Prompt for AI Agents
In GDPR_COMPLIANCE_GUIDE.md around lines 34 to 37, the current wording
incorrectly promises "automatic compliance" and definitive claims for Articles
30/33/32; change these lines to soften the language to say the product or
feature "helps support" or "can assist with" meeting the requirements, that it
"provides tools or an audit trail to help with" Article 33, and that it "can
help provide evidence for" Article 32, so the copy indicates support rather than
guaranteed compliance.
| ```javascript | ||
| // Cloud Code function to export user data | ||
| Parse.Cloud.define('exportMyData', async (request) => { | ||
| const exportData = { | ||
| exportDate: new Date().toISOString(), | ||
| user: request.user.toJSON(), | ||
| relatedData: {} | ||
| }; | ||
|
|
||
| // Query all classes in parallel for better performance | ||
| const [orders, reviews, comments] = await Promise.all([ | ||
| new Parse.Query('Order') | ||
| .equalTo('user', request.user) | ||
| .find({ useMasterKey: true }), | ||
|
|
||
| new Parse.Query('Review') | ||
| .equalTo('author', request.user) | ||
| .find({ useMasterKey: true }), | ||
|
|
||
| new Parse.Query('Comment') | ||
| .equalTo('author', request.user) | ||
| .find({ useMasterKey: true }), | ||
|
|
||
| // Add more queries as needed for your application | ||
| ]); | ||
|
|
||
| exportData.relatedData.orders = orders.map(o => o.toJSON()); | ||
| exportData.relatedData.reviews = reviews.map(r => r.toJSON()); | ||
| exportData.relatedData.comments = comments.map(c => c.toJSON()); | ||
|
|
||
| // Include audit logs for this user | ||
| // (You'll need to query your audit log files) | ||
|
|
||
| return exportData; | ||
| }, { | ||
| requireUser: true | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
Export example: avoid leaking sensitive fields; select explicit user fields
request.user.toJSON() may include fields you don’t intend to export (e.g., sessionToken under some flows or private attrs). Recommend explicit whitelisting.
- const exportData = {
- exportDate: new Date().toISOString(),
- user: request.user.toJSON(),
- relatedData: {}
- };
+ const exportData = {
+ exportDate: new Date().toISOString(),
+ user: {
+ id: request.user.id,
+ username: request.user.get('username'),
+ email: request.user.get('email'),
+ createdAt: request.user.createdAt,
+ updatedAt: request.user.updatedAt,
+ },
+ relatedData: {}
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```javascript | |
| // Cloud Code function to export user data | |
| Parse.Cloud.define('exportMyData', async (request) => { | |
| const exportData = { | |
| exportDate: new Date().toISOString(), | |
| user: request.user.toJSON(), | |
| relatedData: {} | |
| }; | |
| // Query all classes in parallel for better performance | |
| const [orders, reviews, comments] = await Promise.all([ | |
| new Parse.Query('Order') | |
| .equalTo('user', request.user) | |
| .find({ useMasterKey: true }), | |
| new Parse.Query('Review') | |
| .equalTo('author', request.user) | |
| .find({ useMasterKey: true }), | |
| new Parse.Query('Comment') | |
| .equalTo('author', request.user) | |
| .find({ useMasterKey: true }), | |
| // Add more queries as needed for your application | |
| ]); | |
| exportData.relatedData.orders = orders.map(o => o.toJSON()); | |
| exportData.relatedData.reviews = reviews.map(r => r.toJSON()); | |
| exportData.relatedData.comments = comments.map(c => c.toJSON()); | |
| // Include audit logs for this user | |
| // (You'll need to query your audit log files) | |
| return exportData; | |
| }, { | |
| requireUser: true | |
| }); | |
| ``` | |
| // Cloud Code function to export user data | |
| Parse.Cloud.define('exportMyData', async (request) => { | |
| const exportData = { | |
| exportDate: new Date().toISOString(), | |
| user: { | |
| id: request.user.id, | |
| username: request.user.get('username'), | |
| email: request.user.get('email'), | |
| createdAt: request.user.createdAt, | |
| updatedAt: request.user.updatedAt, | |
| }, | |
| relatedData: {} | |
| }; | |
| // Query all classes in parallel for better performance | |
| const [orders, reviews, comments] = await Promise.all([ | |
| new Parse.Query('Order') | |
| .equalTo('user', request.user) | |
| .find({ useMasterKey: true }), | |
| new Parse.Query('Review') | |
| .equalTo('author', request.user) | |
| .find({ useMasterKey: true }), | |
| new Parse.Query('Comment') | |
| .equalTo('author', request.user) | |
| .find({ useMasterKey: true }), | |
| // Add more queries as needed for your application | |
| ]); | |
| exportData.relatedData.orders = orders.map(o => o.toJSON()); | |
| exportData.relatedData.reviews = reviews.map(r => r.toJSON()); | |
| exportData.relatedData.comments = comments.map(c => c.toJSON()); | |
| // Include audit logs for this user | |
| // (You'll need to query your audit log files) | |
| return exportData; | |
| }, { | |
| requireUser: true | |
| }); |
🤖 Prompt for AI Agents
In GDPR_COMPLIANCE_GUIDE.md around lines 79 to 116, the example uses
request.user.toJSON() which can leak sensitive/private fields (e.g.,
sessionToken, internal attrs); replace that call with an explicit whitelist of
user fields to export (e.g., id, username, email, createdAt) by building a new
object from request.user.get(...) or by picking only allowed keys, ensure you
explicitly omit sessionToken and other privateAttrs, and apply the same
explicit-field selection/redaction to related objects (map each Parse.Object to
only the approved fields) before returning exportData.
| // ANONYMIZE financial records (legal requirement to retain) | ||
| for (const order of allOrders) { | ||
| order.set('user', null); | ||
| order.set('userName', 'DELETED_USER'); | ||
| order.set('userEmail', '[email protected]'); | ||
| order.set('userPhone', null); | ||
| await order.save(null, { useMasterKey: true }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Anonymization example: avoid email collisions and pseudo‑PII
Using a fixed value '[email protected]' will violate unique email constraints and can trigger accidental messages. Prefer nulling or generating a non‑contactable, unique placeholder.
- order.set('userName', 'DELETED_USER');
- order.set('userEmail', '[email protected]');
- order.set('userPhone', null);
+ order.set('userName', 'DELETED_USER');
+ order.unset('userEmail'); // or set to a unique nonce if column is required
+ order.set('userPhone', null);Optionally, if email is required:
const nonce = `deleted+${order.id}@example.invalid`; // RFC‑compliant, non‑deliverable TLD
order.set('userEmail', nonce);🤖 Prompt for AI Agents
In GDPR_COMPLIANCE_GUIDE.md around lines 168 to 176, the anonymization example
uses a fixed '[email protected]' which can violate unique email constraints
and risk accidental messaging; change the guidance to either set the email field
to null (preferred if schema allows) or generate a unique, non‑deliverable
placeholder per record (e.g., using the record ID or a nonce and a reserved TLD
like example.invalid) so the value is RFC‑compliant, non‑contactable and unique
across records; update the example text to mention uniqueness and the use of
non‑deliverable domains rather than a single constant.
| const inactiveUsers = await new Parse.Query('_User') | ||
| .lessThan('lastLoginAt', inactiveThreshold) | ||
| .find({ useMasterKey: true }); | ||
|
|
||
| message(`Found ${inactiveUsers.length} inactive users`); | ||
|
|
There was a problem hiding this comment.
Inactive user logic: _User.lastLoginAt is not a default field
Parse Server does not populate lastLoginAt by default. Use _Session to derive last activity or populate a lastLoginAt field via a login hook/audit event.
- const inactiveUsers = await new Parse.Query('_User')
- .lessThan('lastLoginAt', inactiveThreshold)
- .find({ useMasterKey: true });
+ // Derive last activity from sessions (latest session per user older than threshold)
+ const sessions = await new Parse.Query('_Session')
+ .lessThan('createdAt', inactiveThreshold)
+ .include('user')
+ .find({ useMasterKey: true });
+ const inactiveUsers = [...new Set(sessions.map(s => s.get('user')))];Alternatively: maintain lastLoginAt yourself in an auth flow and query that field.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const inactiveUsers = await new Parse.Query('_User') | |
| .lessThan('lastLoginAt', inactiveThreshold) | |
| .find({ useMasterKey: true }); | |
| message(`Found ${inactiveUsers.length} inactive users`); | |
| // Derive last activity from sessions (latest session per user older than threshold) | |
| const sessions = await new Parse.Query('_Session') | |
| .lessThan('createdAt', inactiveThreshold) | |
| .include('user') | |
| .find({ useMasterKey: true }); | |
| const inactiveUsers = [...new Set(sessions.map(s => s.get('user')))]; | |
| message(`Found ${inactiveUsers.length} inactive users`); |
🤖 Prompt for AI Agents
In GDPR_COMPLIANCE_GUIDE.md around lines 382 to 387, the snippet queries
_User.lastLoginAt which is not a default Parse Server field; update the guidance
to either (a) compute last activity from the _Session class (query _Session for
newest session per user and derive lastLoginAt), or (b) instruct implementers to
populate and maintain a lastLoginAt field during login/logout hooks or audit
events and then query that field; clearly show the recommended implementation
approach and example query flow for using _Session or the login hook approach.
| ### Parse Server GDPR Documentation | ||
| - [GDPR_AUDIT_LOGGING.md](./GDPR_AUDIT_LOGGING.md) - Audit logging setup and configuration | ||
|
|
||
| ### External Resources | ||
| - [Official GDPR Text](https://gdpr-info.eu/) - Complete regulation text | ||
| - [ICO GDPR Guide](https://ico.org.uk/for-organisations/guide-to-data-protection/guide-to-the-general-data-protection-regulation-gdpr/) - UK guidance | ||
| - [CNIL GDPR Resources](https://www.cnil.fr/en/home) - French supervisory authority |
There was a problem hiding this comment.
🧩 Analysis chain
Cross‑doc consistency: ensure referenced doc exists and matches options
Verify that GDPR_AUDIT_LOGGING.md exists in this PR and that its option names/examples match this guide.
🏁 Script executed:
#!/bin/bash
# Check doc presence and option consistency
fd -a GDPR_AUDIT_LOGGING.md || true
rg -n "auditLog" GDPR_AUDIT_LOGGING.md -C2 || trueLength of output: 61
🏁 Script executed:
#!/bin/bash
# Search for any markdown files related to audit or GDPR
fd -e md | grep -i audit || true
fd -e md | grep -i gdpr || true
# Search within markdown files for audit logging references
grep -Rin "audit logging" --include "*.md" . || trueLength of output: 819
Fix broken link to GDPR_AUDIT_LOGGING.md
The guide references GDPR_AUDIT_LOGGING.md (GDPR_COMPLIANCE_GUIDE.md line 848), but that file is missing. Add the file or update the link to the correct document.
🤖 Prompt for AI Agents
In GDPR_COMPLIANCE_GUIDE.md around lines 847–853 the markdown links include a
reference to GDPR_AUDIT_LOGGING.md which does not exist; either add a new
GDPR_AUDIT_LOGGING.md file with the audit logging content into the repo (and
commit it alongside updating any frontmatter/indexes), or update the link here
to point to the correct existing document (find the actual audit-logging
filename/path in the repo and replace ./GDPR_AUDIT_LOGGING.md with that path);
ensure the link is relative and valid, and run a link-check or preview to
confirm no broken links remain.
Pull Request
Issue
Closes: #5378
Approach
Tasks
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests