eat: Search by name is supported in cohort member api#738
Conversation
feat: Search by name is supported in cohort member api
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughThis PR adds new filterable fields to cohort member search functionality, extending the searchable name-related fields ( ChangesCohort Member Search Filtering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design specification for PII encryption in the Users table and updates the cohort members service to include additional name fields in search results. However, the current implementation contains several critical issues, including SQL injection vulnerabilities and direct violations of the encryption design. Specifically, the use of partial matches (ILIKE) on encrypted fields and the inclusion of non-searchable fields like middleName in search filters contradict the proposed security strategy. Furthermore, the name field is not yet accounted for in the encryption scope, and required decryption logic is missing from the service layer.
| case "middleName": { | ||
| return `U."middleName" ILIKE '%${value}%'`; | ||
| } | ||
| case "lastName": { | ||
| return `U."lastName" ILIKE '%${value}%'`; | ||
| } | ||
| case "name": { | ||
| return `U."name" ILIKE '%${value}%'`; | ||
| } |
There was a problem hiding this comment.
The implementation of these search filters presents several critical issues:
- SQL Injection: The
valueis directly interpolated into the SQL string. This is a critical security vulnerability. All queries should be parameterized using TypeORM's query parameters. - Design Violation (Searchability): The
middleNamefield is marked as non-searchable in the PII design doc (Section 4) because it uses Random-IV encryption. It should not have a search case here. - Design Violation (Exact Match): The design doc (Section 7.4) states that partial/wildcard search (
ILIKE) is no longer possible for encrypted fields likefirstNameandlastName. These should be updated to use exact matches (=) on the encrypted value. - Missing Decryption: The service logic lacks the required call to
decryptUserPII()after fetching results, which is necessary to return plaintext to the consumer (Section 7.2). - Unaccounted PII: The
namecolumn is not mentioned in the encryption design but contains PII and should be handled accordingly.
| "firstName", | ||
| "middleName", | ||
| "lastName", |
There was a problem hiding this comment.
As per the PII Encryption Design (Section 4 & 11), middleName is encrypted using Random-IV and is not intended to be searchable at the database level. It should be excluded from the whereKeys array.
| "firstName", | |
| "middleName", | |
| "lastName", | |
| "firstName", | |
| "lastName", |
| @IsString() | ||
| lastName?: string; | ||
|
|
||
| @ApiPropertyOptional({ type: String, description: "Partial match on firstName" }) |
There was a problem hiding this comment.
The description for the name filter is misleading as it mentions firstName. It should be updated to accurately describe the field.
| @ApiPropertyOptional({ type: String, description: "Partial match on firstName" }) | |
| @ApiPropertyOptional({ type: String, description: "Partial match on name" }) |
| | Column | DB Column Name (after migration) | Encryption Mode | Searchable in DB? | | ||
| |---|---|---|---| | ||
| | `email` | `email` (same, value encrypted) | Deterministic (AES-256-CBC) | YES | | ||
| | `mobile` | `mobile` (same, value encrypted) | Deterministic (AES-256-CBC) | YES | | ||
| | `dob` | `dob` (same, value encrypted) | Random-IV (AES-256-GCM) | NO | | ||
| | `firstName` | `firstName` (same, value encrypted) | Deterministic (AES-256-CBC) | YES | | ||
| | `lastName` | `lastName` (same, value encrypted) | Deterministic (AES-256-CBC) | YES | | ||
| | `middleName` | `middleName` (same, value encrypted) | Random-IV (AES-256-GCM) | NO | | ||
| | `address` | `address` (same, value encrypted) | Random-IV (AES-256-GCM) | NO | | ||
| | `pincode` | `pincode` (same, value encrypted) | Random-IV (AES-256-GCM) | NO | |
There was a problem hiding this comment.
The Users table contains a name column (as seen in user-entity.ts) which is not addressed in this design document. Since it contains personal data, it should be included in the encryption scope and its searchability defined.
1 participant
Summary by CodeRabbit
New Features
Documentation