fix: normalize user emails on save to strip invisible Unicode characters#41664
Merged
fix: normalize user emails on save to strip invisible Unicode characters#41664
Conversation
Contributor
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailNormalizer.java (2)
8-9: Minor: redundant\u200Cin pattern.
\u200Cis already covered by the range\u200B-\u200D. Not a bug, but can be cleaned up.🧹 Cleaner pattern
private static final Pattern INVISIBLE_CHAR_PATTERN = - Pattern.compile("[\u200B-\u200D\uFEFF\u2060\u00AD\u200C\u200E\u200F\u202A-\u202E\u2061-\u2063]"); + Pattern.compile("[\u200B-\u200D\uFEFF\u2060\u00AD\u200E\u200F\u202A-\u202E\u2061-\u2063]");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailNormalizer.java` around lines 8 - 9, In EmailNormalizer.java, the INVISIBLE_CHAR_PATTERN contains a redundant escape `\u200C` since the range `\u200B-\u200D` already includes it; update the Pattern.compile invocation in the INVISIBLE_CHAR_PATTERN field to remove the extra `\u200C` entry so the regex is cleaner (leave all other code and ranges unchanged), ensuring no duplicate code points remain in the INVISIBLE_CHAR_PATTERN constant.
6-38: Utility class design: consider addingfinaland private constructor.This utility class only has static methods. Per Java conventions, it should be
finalwith a private constructor to prevent instantiation and subclassing.🔧 Proposed fix
-public class EmailNormalizer { +public final class EmailNormalizer { + + private EmailNormalizer() { + // Utility class + } private static final Pattern INVISIBLE_CHAR_PATTERN =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailNormalizer.java` around lines 6 - 38, Make EmailNormalizer a non-instantiable utility class by declaring the class final and adding a private constructor (e.g., private EmailNormalizer() { throw new AssertionError("No instances."); }) to prevent instantiation and subclassing; update the class declaration for EmailNormalizer and add the private constructor while leaving the existing static methods normalizeEmail and containsInvisibleCharacters unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java`:
- Line 466: EmailNormalizer.normalizeEmail(user.getEmail()) can return null for
emails that become empty after stripping invisible characters; update the
UserServiceCEImpl flow around the call that currently does
user.setEmail(EmailNormalizer.normalizeEmail(user.getEmail())) to guard against
null: call EmailNormalizer.normalizeEmail(...) into a local variable, validate
it is non-null/non-empty, and if null either throw a clear
IllegalArgumentException (or a domain-specific BadRequestException) with a
descriptive message or handle it by setting a safe default, so downstream
methods and repository lookups that rely on user.getEmail() never receive null.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java`:
- Around line 346-365: In the disabled-user re-enable path in UserServiceCEImpl
(the block around the re-enable flow currently calling repository.save()
directly, lines ~527-540), normalize the user's email with
EmailNormalizer.normalizeEmail(...) before persisting instead of relying on
create(); i.e., compute String normalized =
EmailNormalizer.normalizeEmail(user.getEmail()), set it back on the User
(user.setEmail(normalized)) and then call repository.save(user) so the re-enable
flow stores the cleaned email consistently with create().
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailNormalizer.java`:
- Around line 8-9: In EmailNormalizer.java, the INVISIBLE_CHAR_PATTERN contains
a redundant escape `\u200C` since the range `\u200B-\u200D` already includes it;
update the Pattern.compile invocation in the INVISIBLE_CHAR_PATTERN field to
remove the extra `\u200C` entry so the regex is cleaner (leave all other code
and ranges unchanged), ensuring no duplicate code points remain in the
INVISIBLE_CHAR_PATTERN constant.
- Around line 6-38: Make EmailNormalizer a non-instantiable utility class by
declaring the class final and adding a private constructor (e.g., private
EmailNormalizer() { throw new AssertionError("No instances."); }) to prevent
instantiation and subclassing; update the class declaration for EmailNormalizer
and add the private constructor while leaving the existing static methods
normalizeEmail and containsInvisibleCharacters unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0de7dd3e-7857-4ed2-9309-463747c17723
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/EmailNormalizer.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/EmailNormalizerTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java
|
|
||
| // convert the user email to lowercase | ||
| user.setEmail(user.getEmail().toLowerCase()); | ||
| user.setEmail(EmailNormalizer.normalizeEmail(user.getEmail())); |
Contributor
There was a problem hiding this comment.
Potential NPE if email consists only of invisible characters.
EmailNormalizer.normalizeEmail() returns null when the email is empty after stripping invisible chars. Setting user.setEmail(null) could cause downstream failures (validation, repository lookups, etc.). Consider adding a guard or throwing an explicit exception.
🛡️ Proposed defensive check
- user.setEmail(EmailNormalizer.normalizeEmail(user.getEmail()));
+ String normalizedEmail = EmailNormalizer.normalizeEmail(user.getEmail());
+ if (normalizedEmail == null) {
+ throw new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.EMAIL);
+ }
+ user.setEmail(normalizedEmail);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java`
at line 466, EmailNormalizer.normalizeEmail(user.getEmail()) can return null for
emails that become empty after stripping invisible characters; update the
UserServiceCEImpl flow around the call that currently does
user.setEmail(EmailNormalizer.normalizeEmail(user.getEmail())) to guard against
null: call EmailNormalizer.normalizeEmail(...) into a local variable, validate
it is non-null/non-empty, and if null either throw a clear
IllegalArgumentException (or a domain-specific BadRequestException) with a
descriptive message or handle it by setting a safe default, so downstream
methods and repository lookups that rely on user.getEmail() never receive null.
Comment on lines
+346
to
+365
| @Test | ||
| @WithMockAppsmithUser | ||
| public void signUpViaGoogle_WhenEmailHasInvisibleUnicode_SavedNormalized() { | ||
| String dirtyEmail = "\u2060google-signup-dirty@example.com"; | ||
| String expectedCleanEmail = "google-signup-dirty@example.com"; | ||
|
|
||
| User signupUser = new User(); | ||
| signupUser.setEmail(dirtyEmail); | ||
| signupUser.setPassword("test-password-123"); | ||
| signupUser.setSource(LoginSource.GOOGLE); | ||
|
|
||
| Mono<User> userMono = userService.create(signupUser); | ||
|
|
||
| StepVerifier.create(userMono) | ||
| .assertNext(user -> { | ||
| assertThat(user.getEmail()).isEqualTo(expectedCleanEmail); | ||
| assertThat(user.getSource()).isEqualTo(LoginSource.GOOGLE); | ||
| }) | ||
| .verifyComplete(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ReactiveBeforeSaveCallback implementations for User
ast-grep --pattern $'class $_ implements ReactiveBeforeSaveCallback<User> {
$$$
}'
# Also search for BeforeConvertCallback or similar Spring Data callbacks
rg -n --type=java 'BeforeSaveCallback|BeforeConvertCallback' -A5Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# First, locate the UserServiceTest.java file
fd -t f "UserServiceTest.java"Repository: appsmithorg/appsmith
Length of output: 154
🏁 Script executed:
# Read the test method at lines 346-365
head -365 app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java | tail -20Repository: appsmithorg/appsmith
Length of output: 862
🏁 Script executed:
# Search for UserServiceCEImpl to understand the re-enable path
fd -t f "UserServiceCEImpl.java"Repository: appsmithorg/appsmith
Length of output: 506
🏁 Script executed:
# Search for email normalization logic
rg -n --type=java 'normalize|clean.*email|invisible.*unicode' -B2 -A2Repository: appsmithorg/appsmith
Length of output: 50376
🏁 Script executed:
# Search for all User save operations and potential callbacks
rg -n --type=java '\.save\(.*User|ReactiveBeforeSaveCallback|BeforeSaveCallback|BeforeConvertCallback' -B3 -A3Repository: appsmithorg/appsmith
Length of output: 12706
🏁 Script executed:
# Read UserServiceCEImpl.java around the re-enable path (lines 520-545)
sed -n '520,545p' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaRepository: appsmithorg/appsmith
Length of output: 1604
🏁 Script executed:
# Also check if there are any tests that cover the re-enable path with dirty emails
rg -n --type=java 'signUpViaGoogle_WhenEmailHasInvisibleUnicode|isEnabled.*false' -B5 -A10 app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.javaRepository: appsmithorg/appsmith
Length of output: 673
Normalization gap in disabled-user re-enable path.
When a disabled user signs up again via the re-enable flow (UserServiceCEImpl:527-540), the email is not normalized before saving. The create() method normalizes via EmailNormalizer.normalizeEmail(), but the re-enable path directly calls repository.save() without this step. Add email normalization in the re-enable flow before persisting.
Note: The callback verification mentioned in the original comment is unnecessary—no ReactiveBeforeSaveCallback<User> implementation exists. The normalization must be added explicitly to the re-enable code path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java`
around lines 346 - 365, In the disabled-user re-enable path in UserServiceCEImpl
(the block around the re-enable flow currently calling repository.save()
directly, lines ~527-540), normalize the user's email with
EmailNormalizer.normalizeEmail(...) before persisting instead of relying on
create(); i.e., compute String normalized =
EmailNormalizer.normalizeEmail(user.getEmail()), set it back on the User
(user.setEmail(normalized)) and then call repository.save(user) so the re-enable
flow stores the cleaned email consistently with create().
sondermanish
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/8830
Summary
EmailNormalizerutility to strip invisible Unicode characters (U+2060, U+200B, etc.), apply NFKC normalization, trim, and lowercase emailsReactiveBeforeSaveCallback<User>that normalizes emails on every User save, regardless of entry pointThis is a defense-in-depth measure complementing the cloud-services fixes in APP-15054 and APP-15055.
Slack Thread
https://theappsmith.slack.com/archives/C0341RERY4R/p1774364105450899?thread_ts=1767798360.977899&cid=C0341RERY4R
Test Plan
EmailNormalizer(invisible char stripping, idempotency, null handling)UserBeforeSaveCallback(normalization, collision skip, clean email no-op, non-user collection no-op)Summary by CodeRabbit
Automation
/ok-to-test tags="@tag.All"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/23641217711
Commit: 9bc4b37
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 27 Mar 2026 11:24:48 UTC