Anonymize member creation for activities if listed in erasure requests #2711
Conversation
WalkthroughThis pull request introduces changes to the database schema and application logic related to member identity anonymization. It modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
services/apps/data_sink_worker/src/bin/anonymize-member.ts (1)
256-256: Address TODO: Implement member data cleanup inanonymizeMemberInDbThere is a TODO comment indicating that original member data in the
memberstable should be cleaned up. This is essential for complete anonymization.Would you like assistance in implementing the cleanup of original member data to ensure full compliance with anonymization requirements?
backend/src/database/migrations/U1733322265__anonymizeMemberIdentities.sql (1)
1-3: Verify the necessity of modifying the unique constraint in separate migrationsThe unique constraint
unique_anonymized_memberis dropped and re-added includingmemberId. Ensure that running these migrations separately doesn't cause issues and consider combining them for consistency.Consider merging this migration with
V1733322265__anonymizeMemberIdentities.sqlto streamline the schema changes.services/libs/data-access-layer/src/gdpr/index.ts (1)
3-9: Enhance documentation with GDPR compliance and security details.While the current documentation is clear, consider adding important details about:
- GDPR compliance aspects
- The irreversible nature of the anonymization
- Security implications of the 8-character hash length
Consider updating the documentation:
/** * Creates a deterministic anonymized username from the original username, platform and type + * This function implements GDPR-compliant irreversible anonymization. * @param username The original username to anonymize * @param platform The platform the username belongs to * @param type The type of identity (e.g. 'username', 'email') * @returns A consistently hashed anonymous identity in the format 'anon_<type>_<hash>' + * @remarks + * - The anonymization is irreversible due to one-way SHA-256 hashing + * - Uses first 8 characters of the hash for brevity while maintaining uniqueness + * - Deterministic output ensures consistent anonymization across requests */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
backend/src/database/migrations/U1733322265__anonymizeMemberIdentities.sql(1 hunks)backend/src/database/migrations/V1733322265__anonymizeMemberIdentities.sql(1 hunks)services/apps/data_sink_worker/src/bin/anonymize-member.ts(1 hunks)services/apps/data_sink_worker/src/service/activity.service.ts(4 hunks)services/libs/data-access-layer/src/gdpr/index.ts(1 hunks)services/libs/data-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts(2 hunks)
🔇 Additional comments (3)
services/libs/data-access-layer/src/gdpr/index.ts (3)
1-1: LGTM! Using Node.js built-in crypto module is appropriate.
The choice of the built-in crypto module ensures reliable cryptographic operations.
10-22: Verify GDPR compliance aspects.
Please ensure:
- The anonymization is consistently applied across all member-related operations
- No PII is leaked in logs or error messages when anonymization fails
Let's verify these aspects:
✅ Verification successful
Based on the search results, I can provide a final response:
GDPR compliance aspects verified successfully
-
The anonymization is consistently applied across member-related operations:
- Used in data sink worker for activity processing
- Implemented in member erasure and anonymization scripts
- Properly handles username, platform, and type parameters
-
No direct PII leakage found in error handling:
- Error messages use generic terms without exposing raw usernames
- Anonymization happens before any logging or error reporting
- The anonymization function is deterministic and one-way using SHA-256
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent anonymization usage
rg -A 5 "anonymizeUsername" --type typescript
# Look for potential PII leakage in error handling
rg -A 5 "catch|error|throw" --type typescript | rg -i "username|email|identity"
# Check logging statements around anonymization
rg "console\.|logger\." --type typescript | rg -i "username|email|identity"
Length of output: 310
Script:
#!/bin/bash
# Check for consistent anonymization usage across the codebase
rg -A 5 "anonymizeUsername"
# Look for potential PII leakage in error handling
rg -A 5 "catch|error|throw" | rg -i "username|email|identity"
# Check logging statements that might contain PII
rg "console\.|logger\." | rg -i "username|email|identity"
# Find all files that might handle user data
fd -e ts -e js | xargs rg -l "username|email|identity"
# Check for other anonymization functions
rg "anonymize|hash.*user"
Length of output: 70753
10-22: Consider collision risks with 8-character hash truncation.
While the implementation is generally solid, the 8-character hash truncation (32 bits) might lead to collisions in large datasets. The birthday paradox suggests potential collisions after approximately 2^16 entries.
Consider:
- Increasing the hash length to at least 12 characters (48 bits) for better uniqueness
- Adding collision detection and resolution mechanism
Let's verify the usage and potential collision risks:
✅ Verification successful
Based on the search results, I can now provide a final response:
8-character hash truncation is acceptable for this use case
The implementation of anonymizeUsername is used primarily for GDPR-related anonymization of individual usernames, not for bulk operations. The code analysis shows:
- Usage is limited to specific GDPR erasure requests in
data_sink_worker - Anonymization happens one user at a time through:
- Individual erasure requests (
getAnonymizationRequest) - Single-member anonymization processes (
anonymizeMemberIfRequested)
- Individual erasure requests (
- No evidence of bulk anonymization that could lead to collision risks
The current 8-character hash (32 bits) provides sufficient uniqueness for this specific GDPR anonymization use case, where:
- Collisions would be extremely rare due to low volume
- Each anonymization includes platform and type in hash calculation
- The primary goal is consistent one-way anonymization, not unique identification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential collision scenarios in the codebase
# Look for large batch operations or loops that might create many anonymous identities
# Search for bulk member operations
rg -A 5 "members?.create|members?.update" --type typescript
# Look for array operations that might involve member anonymization
ast-grep --pattern 'members.map($$$)'
# Check if there's existing collision handling
rg -i "collision|duplicate|conflict" --type typescript
Length of output: 7547
Script:
#!/bin/bash
# Let's search for actual usage of anonymizeUsername function and any batch processing
rg -t ts -A 5 "anonymizeUsername"
# Look for any GDPR or anonymization related batch operations
rg -t ts -A 5 "anonymize|gdpr"
# Check if there are any large member lists or bulk operations
rg -t ts -A 5 "bulkCreate|bulkUpdate|insertMany|updateMany"
Length of output: 27226
...-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts
Show resolved
Hide resolved
...-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts
Show resolved
Hide resolved
...-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts
Outdated
Show resolved
Hide resolved
backend/src/database/migrations/V1733322265__anonymizeMemberIdentities.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
services/libs/data-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts (1)
61-79: Add error handling for database operationsThe method should handle potential database errors gracefully to prevent silent failures.
private async insertIdentityForErasureRequest({ platform, type, value, memberId, }): Promise<void> { + try { const query = ` insert into "requestedForErasureMemberIdentities" (id, platform, type, value, memberId) values ($(id), $(platform), $(type), $(value), $(memberId)) ` return await this.db().none(query, { id: generateUUIDv1(), platform, type, value, memberId, }) + } catch (error) { + this.log.error(error, 'Failed to insert identity for erasure request') + throw error + } }services/apps/data_sink_worker/src/bin/anonymize-member.ts (2)
22-33: Add input validation for CLI argumentsWhile the code checks for argument pairs, it should also validate the content of these arguments (e.g., email format, platform values).
+const VALID_PLATFORMS = ['github', 'gitlab', 'lfid'] // Add all valid platforms + +function validateEmail(email: string): boolean { + return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email) +} + if (processArguments.length === 0 || processArguments.length % 2 !== 0) { log.error( ` Expected argument in pairs which can be any of the following: - ids "<memberId1>, <memberId2>, ..." - email [email protected] - name "John Doe" - <platform> <value> (e.g. lfid someusername) `, ) process.exit(1) } + +// Validate argument values +for (let i = 0; i < processArguments.length; i += 2) { + const type = processArguments[i] + const value = processArguments[i + 1] + + if (type === 'email' && !validateEmail(value)) { + log.error(`Invalid email format: ${value}`) + process.exit(1) + } + + if (!['ids', 'email', 'name'].includes(type) && !VALID_PLATFORMS.includes(type)) { + log.error(`Invalid argument type: ${type}`) + process.exit(1) + } +}
256-256: Address the TODO comment about member data cleanupThe comment indicates a need for cleanup of original member data in the members table. This should be addressed to prevent data leakage.
Would you like me to help implement the cleanup logic for the original member data?
services/apps/data_sink_worker/src/service/activity.service.ts (1)
Line range hint
1011-1086: Add error handling for erasure request updatesThe erasure request updates should be wrapped in try-catch blocks to handle potential failures gracefully.
// If this was an anonymized member, update the erasure table with the new memberId if (anonymizedMember) { + try { for (const identity of member.identities) { await erasureRepo.updateErasureRequestMemberId(identity, memberId) } + } catch (error) { + log.error(error, { memberId }, 'Failed to update erasure requests for anonymized member') + throw error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
backend/src/database/migrations/V1733322265__anonymizeMemberIdentities.sql(1 hunks)services/apps/data_sink_worker/src/bin/anonymize-member.ts(1 hunks)services/apps/data_sink_worker/src/service/activity.service.ts(4 hunks)services/libs/data-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/database/migrations/V1733322265__anonymizeMemberIdentities.sql
🔇 Additional comments (4)
services/libs/data-access-layer/src/old/apps/data_sink_worker/repo/requestedForErasureMemberIdentities.repo.ts (3)
81-95: Include platform and type in WHERE clause
To ensure the correct record is updated and prevent unintended updates across different platforms or types, include platform and type in the WHERE clause.
97-130: Fix return value mismatch in getAnonymizationRequest method
The method returns an object with properties id, memberId, and value, but the SELECT query only fetches id and memberId.
132-188: Preserve existing member attributes in anonymizeMemberIfRequested
Setting attributes to an empty object may unintentionally remove important non-sensitive member attributes.
services/apps/data_sink_worker/src/service/activity.service.ts (1)
446-476: LGTM! Well-structured anonymization logic
The implementation properly handles member anonymization with appropriate logging and display name updates.
| function addLinesToFile(filePath: string, lines: string[]) { | ||
| try { | ||
| fs.mkdirSync(path.dirname(filePath), { recursive: true }) | ||
| try { | ||
| fs.accessSync(filePath) | ||
| fs.appendFileSync(filePath, lines.join('\n') + '\n') | ||
| } catch (error) { | ||
| fs.writeFileSync(filePath, lines.join('\n') + '\n') | ||
| } | ||
| } catch (err) { | ||
| log.error(err, { filePath }, 'Error while writing to file!') | ||
| throw err | ||
| } | ||
| } |
There was a problem hiding this comment.
Add path validation in addLinesToFile
The function should validate the file path to prevent directory traversal attacks.
function addLinesToFile(filePath: string, lines: string[]) {
try {
+ // Validate file path
+ const normalizedPath = path.normalize(filePath)
+ if (normalizedPath.includes('..')) {
+ throw new Error('Directory traversal detected in file path')
+ }
+
fs.mkdirSync(path.dirname(filePath), { recursive: true })
try {
fs.accessSync(filePath)Committable suggestion skipped: line range outside the PR's diff.
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes