Conversation
Greptile OverviewGreptile SummaryThis PR enforces lowercase email addresses across the stack:
Blocking issues to address before merge:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant FE as Frontend
participant API as DataArgProcessor
participant EmailsV as validateEmailsFieldOrThrow
participant EmailV as validateEmailValueOrThrow
participant AddArrV as validateAdditionalEmailArrayOrThrow
participant GQL as WorkspaceQueryRunner
participant Assert as assertIsValidEmail
FE->>FE: emailSchema (z.email().lowercase())
FE->>API: submit emails payload
API->>EmailsV: validateEmailsFieldOrThrow(value, fieldName)
EmailsV->>EmailV: validate primaryEmail (if string)
EmailsV->>AddArrV: validate additionalEmails
AddArrV->>EmailV: validate each additional email
GQL->>Assert: assertIsValidEmail(value)
Assert-->>GQL: throws WorkspaceQueryRunnerException if invalid
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export const validateAdditionalEmailArrayOrThrow = ( | ||
| value: unknown, | ||
| fieldName: string, | ||
| ): string | string[] | null => { |
There was a problem hiding this comment.
Return type doesn’t match data shape
validateAdditionalEmailArrayOrThrow is used to validate the additionalEmails subfield (which is typed as string[] | null in validateEmailsFieldOrThrow), but it currently declares it can return string | string[] | null and even returns a plain string when the input is a string (typeof value === 'string' branch). That means invalid shapes can slip through type-wise and the validator can silently accept a string for a field that should be an array. This should return string[] | null and reject non-arrays rather than accepting/returning a string.
| case 'primaryEmail': | ||
| validateTextFieldOrThrow(subFieldValue, `${fieldName}.${subField}`); | ||
| if (typeof subFieldValue === 'string' && subFieldValue) { | ||
| validateEmailValueOrThrow(subFieldValue, `${fieldName}.${subField}`); | ||
| } |
There was a problem hiding this comment.
Non-string primaryEmail passes
In the primaryEmail branch, non-empty values that are not strings (e.g. primaryEmail: 12345) won’t throw because the code only validates when typeof subFieldValue === 'string' && subFieldValue. Previously this was rejected via validateTextFieldOrThrow. This breaks the existing test case expecting an exception for non-string primaryEmail.
| if (typeof value === 'string') { | ||
| validateEmailValueOrThrow(value, fieldName); | ||
| } | ||
|
|
||
| if ( | ||
| !Array.isArray(value) || | ||
| value.some((item) => !validateEmailValueOrThrow(item, fieldName)) |
There was a problem hiding this comment.
String input accepted for array field
This util is supposed to validate an array of additional emails, but it explicitly accepts typeof value === 'string' and only validates it (without throwing) before continuing. Since Array.isArray(value) will be false, it will still throw later, but the declared return type and this branch suggest strings are acceptable. More importantly, the error message thrown refers to “Array values need to be string” but the real issue is “value must be an array of strings”. Consider removing the string branch and making the thrown message match the actual constraint.
| if ( | ||
| !Array.isArray(value) || | ||
| value.some((item) => !validateEmailValueOrThrow(item, fieldName)) | ||
| ) { | ||
| const inspectedValue = inspect(value); | ||
|
|
||
| throw new CommonQueryRunnerException( | ||
| `Invalid value ${inspectedValue} for field "${fieldName} - Array values need to be string"`, | ||
| CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA, |
There was a problem hiding this comment.
Runtime type hole in some()
value.some((item) => !validateEmailValueOrThrow(item, fieldName)) passes item (type unknown) into validateEmailValueOrThrow(value: string, ...). If the array contains non-strings, validateEmailValueOrThrow will attempt value.toLowerCase() and throw a TypeError rather than a CommonQueryRunnerException, changing error categorization and likely failing expected error handling. You need an explicit typeof item === 'string' check before calling the email validator.
| export const assertIsValidEmail = (value: string) => { | ||
| if (!z.email().lowercase().safeParse(value).success) { | ||
| throw new WorkspaceQueryRunnerException( | ||
| `Value "${value}" is not a valid email`, | ||
| WorkspaceQueryRunnerExceptionCode.INVALID_QUERY_INPUT, | ||
| { userFriendlyMessage: msg`Invalid email format.` }, | ||
| ); |
There was a problem hiding this comment.
Interpolation in error message
The exception message uses Value "${value}" is not a valid email inside a normal string literal, so it will literally include ${value} rather than the actual email. This makes debugging harder and breaks parity with other error messages that include the actual invalid value.
| primaryEmail: z | ||
| .object({ | ||
| eq: z | ||
| .string() | ||
| .email() | ||
| .lowercase() | ||
| .optional() | ||
| .describe('Primary email equals'), | ||
| neq: z | ||
| .string() | ||
| .email() | ||
| .lowercase() | ||
| .optional() |
There was a problem hiding this comment.
Email filter schema type regression
In the EMAILS filter schema, eq/neq were changed from z.string().email() to z.email().lowercase(). Dropping .string() likely changes the schema type/behavior (and may not exist depending on the Zod version in this repo), which can break filter parsing/validation at runtime. This should remain a string schema with .email().lowercase() chained on it.
There was a problem hiding this comment.
Pull request overview
This PR aims to enforce/validate lowercase email handling across the frontend and backend to prevent case-related mismatches (notably impacting imports and filtering/querying by email).
Changes:
- Added lowercase enforcement to email filter schemas and frontend email validation schema.
- Introduced server-side utilities to validate email values (including lowercase constraint) and validate additional email arrays.
- Expanded unit tests for
validateEmailsFieldOrThrowto cover uppercase email inputs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-server/src/engine/core-modules/record-crud/zod-schemas/field-filters.zod-schema.ts | Enforces lowercase email values for eq/neq email filters. |
| packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/assert-is-valid-email.util.ts | Adds a workspace query-runner helper to assert email validity with lowercase constraint. |
| packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-field-or-throw.util.ts | Refactors emails-field validation to use new email-specific validators. |
| packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-email-value-or-throw.util.ts | Adds a dedicated validator for a single email value (incl. lowercase). |
| packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts | Adds validation for additionalEmails array contents (incl. lowercase). |
| packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/tests/validate-emails-field-or-throw.util.spec.ts | Adds test cases for uppercase primaryEmail and additionalEmails. |
| packages/twenty-front/src/modules/object-record/record-field/ui/validation-schemas/emailSchema.ts | Enforces lowercase in frontend email schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...on-args-processors/data-arg-processor/validator-utils/validate-emails-field-or-throw.util.ts
Outdated
Show resolved
Hide resolved
| if (typeof value === 'string') { | ||
| validateEmailValueOrThrow(value, fieldName); | ||
| } | ||
|
|
||
| if ( | ||
| !Array.isArray(value) || | ||
| value.some((item) => !validateEmailValueOrThrow(item, fieldName)) | ||
| ) { |
There was a problem hiding this comment.
When additionalEmails is provided as a string, this function validates it but then continues and immediately throws because value is not an array. If string inputs are meant to be accepted (as validateArrayFieldOrThrow did), return early after validating the string; otherwise, drop the string branch and update the return type accordingly.
...ocessors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts
Outdated
Show resolved
Hide resolved
| export const validateEmailValueOrThrow = ( | ||
| value: string, | ||
| fieldName: string, | ||
| ): string | null => { | ||
| if (isNull(value)) return null; | ||
| if ( | ||
| value !== value.toLowerCase() || | ||
| !z.email().trim().safeParse(value).success | ||
| ) { |
There was a problem hiding this comment.
validateEmailValueOrThrow is typed to accept value: string, but it contains null-handling and is invoked from codepaths where the input may not be a string (e.g. unknown JSON array items). As written, non-string inputs can cause runtime errors (e.g. value.toLowerCase is not a function) instead of a controlled CommonQueryRunnerException. Consider changing the signature to value: unknown and performing a typeof value === 'string' check up front (mirroring validateTextFieldOrThrow).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts:18">
P2: String inputs are validated but not returned, so they fall through to the array check and always throw. This makes valid string inputs fail despite the function signature allowing `string`.</violation>
<violation number="2" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts:24">
P1: Missing type check before calling `validateEmailValueOrThrow`. If the array contains non-string items (e.g., numbers), calling `value.toLowerCase()` inside the validator will throw a `TypeError` rather than the expected `CommonQueryRunnerException`. Add a `typeof item === 'string'` check before calling the validator.</violation>
</file>
<file name="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-field-or-throw.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-field-or-throw.util.ts:26">
P2: Non-string primaryEmail values now bypass validation. The new conditional only validates non-empty strings and no longer enforces that non-null values are strings, so numbers/booleans can slip through and be returned as `primaryEmail`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...ocessors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts
Outdated
Show resolved
Hide resolved
...ocessors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts
Show resolved
Hide resolved
...on-args-processors/data-arg-processor/validator-utils/validate-emails-field-or-throw.util.ts
Outdated
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:13358 This environment will automatically shut down when the PR is closed or after 5 hours. |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts:24">
P2: Array validation now uses `forEach`, which always returns undefined, so the error block is never reached for arrays. Because `validateEmailValueOrThrow` returns null for null items (without throwing), arrays containing null values now pass validation even though only strings should be allowed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...ocessors/data-arg-processor/validator-utils/validate-additional-email-array-or-throw.util.ts
Outdated
Show resolved
Hide resolved
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
...base/commands/upgrade-version-command/1-17/1-17-transform-all-emails-to-lowercase.command.ts
Outdated
Show resolved
Hide resolved
| } from 'src/engine/api/common/common-query-runners/errors/common-query-runner.exception'; | ||
| import { validateEmailValueOrThrow } from 'src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-email-value-or-throw.util'; | ||
|
|
||
| export const validateAdditionalEmailArrayOrThrow = ( |
There was a problem hiding this comment.
shouldn't we rather convert to lowercase instead of throwing @BOHEUS ?
There was a problem hiding this comment.
I'm not entirely sure about the approach whether it's better to throw an error to user or simply convert it, I just went with the former as I think it's better to inform user about an error and let them fix on their own rather than converting it to lowercase and having user surprised why email is changed
Speaking of converting to lowercase, there's already a convertion in transform-emails-value.util.ts
Let me know which approach is better and I'll make according changes
There was a problem hiding this comment.
I'm lost, so is the lowercasing already in place? It seems so as you mentioned the transform-emails-value.util.ts
I don't think we should block honestly, we are normalizing the input which makes sense
There was a problem hiding this comment.
Basically:
- lowercasing is already in place in transform-emails-value.util.ts (I missed it earlier focusing mainly on the issue and validation)
- there's no validation whether email is lowercase or not (which can be redundant if there's already lowercasing in place as I'm thinking now but I still think it's better to inform user that they should change it on their own and remove lowercasing)
- there's no validation whether email is really a email meaning if someone creates/updates a record via API with some kind of string instead of email, it'll be accepted instead of rejected (front has a validation so only backend is affected and it must be fixed)
- lowercasing email has been implemented almost 3 months ago from what I can see (Common - Field validation #15491) so I'm not sure if earlier emails were lowercased by default or not, I'd assume they weren't
Given everything, should I remove lowercase validation (allowing emails to be lowercased by default) or remove default lowercasing? Email format validation must be added as well as migration command to run to ensure that all emails are really lowercase, please let me know which changes should I implement
There was a problem hiding this comment.
I think I remember that a command to lowercase was actually run
So I would say, we can have:
- keep the lowercasing we already have in place
- no need for the command
- let's use validation to block non valid emails => here I wonder if it should happen at a different / same place than the lowercasing. It feels weird to validate, then lowercase, I feel it's the same process @etiennejouan what do you think?
There was a problem hiding this comment.
I'm not sure we want to reject uppercase email. Data arg processor validate and transform each fields. Concerning EMAILS, it check the emails object format then it transforms with transformEmailsValue (lowercasing all emails - then no need for this PR's migration).
@BOHEUS when checking the issues you link to this PR, I think this one is relevant (the others should have been closed). We should have the same transformation on filter value to be sure condition matches. I'll include it in this sprint
There was a problem hiding this comment.
Thanks @etiennejouan, should I apply changes proposed by Charles or do something more than that?
There was a problem hiding this comment.
I think that changes proposed by Charles are already live. I'll fix filtering issues on email. Then, I'm sorry but I think we should close this PR. Do not hesitate to reach me it seems unclear.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/constants/failing-create-input-by-field-metadata-type.constant.ts">
<violation number="1" location="packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/constants/failing-create-input-by-field-metadata-type.constant.ts:350">
P2: `additionalEmailsField` is not defined in the test object metadata; these failing EMAILS cases target a non-existent field and will not validate email formatting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...lidation/create-validation/constants/failing-create-input-by-field-metadata-type.constant.ts
Outdated
Show resolved
Hide resolved
etiennejouan
left a comment
etiennejouan
left a comment
There was a problem hiding this comment.
Thanks @BOHEUS nice PR and good catch ! 🙏
I comment the change I made, feel free to reach me if not clear.
| import { validateEmailValueOrThrow } from 'src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-email-value-or-throw.util'; | ||
|
|
||
| export const validateAdditionalEmailArrayOrThrow = ( | ||
| export const validateEmailsAdditionalEmailsSubfieldOrThrow = ( |
There was a problem hiding this comment.
While renaming, you probably duplicated a file by accident, I deleted it
...s/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts
Outdated
Show resolved
Hide resolved
|
|
||
| exports[`Create input validation - EMAILS Gql create input - failure EMAILS - should fail with : {"emailsField":"not-an-email"} 1`] = `"Expected type "EmailsCreateInput" to be an object."`; | ||
|
|
||
| exports[`Create input validation - EMAILS Gql create input - failure EMAILS - should fail with : {"emailsField":{"additionalEmails":"not-an-email"}} 1`] = `"Invalid string value 'not-an-email' for email field "emailsField.additionalEmails""`; |
There was a problem hiding this comment.
when updating integration test, you can launch this command to be sure test is ok + to update snapshot if needed
There was a problem hiding this comment.
Could you please write down a command I can run to check if test is OK? I don't have such dropdown in my IDE
There was a problem hiding this comment.
nx run twenty-server:jest --config ./jest-integration.config.ts packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/emails-field-create-input-validation.integration-spec.ts --silent=false --updateSnapshot
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-additional-emails-subfield-or-throw.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-additional-emails-subfield-or-throw.util.ts:34">
P2: userFriendlyMessage repeats inspected technical details; per guidance it should stay generic and avoid duplicating the internal error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| throw new CommonQueryRunnerException( | ||
| `Invalid value ${inspectedValue} for field "${fieldName} - Array values need to be string"`, | ||
| CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA, | ||
| { userFriendlyMessage: msg`Invalid value: "${inspectedValue}"` }, |
There was a problem hiding this comment.
P2: userFriendlyMessage repeats inspected technical details; per guidance it should stay generic and avoid duplicating the internal error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-additional-emails-subfield-or-throw.util.ts, line 34:
<comment>userFriendlyMessage repeats inspected technical details; per guidance it should stay generic and avoid duplicating the internal error.</comment>
<file context>
@@ -0,0 +1,39 @@
+ throw new CommonQueryRunnerException(
+ `Invalid value ${inspectedValue} for field "${fieldName} - Array values need to be string"`,
+ CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA,
+ { userFriendlyMessage: msg`Invalid value: "${inspectedValue}"` },
+ );
+ }
</file context>
...s/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts
Outdated
Show resolved
Hide resolved
...ta-arg-processor/validator-utils/validate-emails-additional-emails-subfield-or-throw.util.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts:16">
P2: Non-string inputs now return null instead of throwing, making the explicit type check unreachable and weakening validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...s/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if ( | ||
| !z.email({ pattern: z.regexes.unicodeEmail }).safeParse(value).success && |
There was a problem hiding this comment.
Bug: The code uses z.regexes.unicodeEmail, but z.regexes is not a valid property in the Zod v4 API, which will cause a runtime TypeError during validation.
Severity: CRITICAL
Suggested Fix
Remove the { pattern: z.regexes.unicodeEmail } argument from the z.email() call. The default email validation provided by Zod is generally sufficient. If a specific unicode-aware regex is required, it should be defined locally and passed as the pattern.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts#L29
Potential issue: The validation logic on line 29 calls `z.email({ pattern:
z.regexes.unicodeEmail })`. However, the `z.regexes` object does not exist in the
version of Zod being used (v4.1.11). This will result in a `TypeError` at runtime when
the validation function is invoked, as it attempts to access the `unicodeEmail` property
of an `undefined` value. This error will occur whenever email validation is performed on
email fields during data mutations, causing the server request to fail.
LogDetails |
|
Hey @etiennejouan! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 similar comment
|
Hey @etiennejouan! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
Fixes twentyhq/core-team-issues#120 #16976
Partially related to #17711
Frontend check surprisingly was one-liner covering both email input and import files
Migration script will be done in next commit