[PM-27541] Add optional marketing param to email verification link

[rr-bw]

🎟️ Tracking

PM-27541

Stacked on top of:

• [PM-27540] Add optional Marketing Property to RegisterSendVerificationEmailRequestModel #6598

📔 Objective

Adds an optional &fromMarketing=premium query parameter to the verification email link.

Feature flag: "pm-26140-marketing-initiated-premium-flow"

📸 Screenshots

For this test, instead of sending via the Marketing site form, I locally adjusted the request in clients to include fromMarketing: "premium" in the POST to /accounts/register/send/verification-email

Feature flags on for this test:

• Server: "pm-26140-marketing-initiated-premium-flow"
• Client: "pm-24033-updat-premium-subscription-page"

test-marketing-initiated-premium-flow.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – a6ae578d-becf-4a97-82cc-e825f3f389e3

Great job! No new security vulnerabilities introduced in this pull request

[claude]

Claude finished @rr-bw's task —— View job

---

Code Review Complete

Summary

This PR adds an optional fromMarketing query parameter to the email verification link, controlled by the pm-26140-marketing-initiated-premium-flow feature flag. The implementation is clean and well-tested. I identified one critical security issue, one minor improvement opportunity, and noted several good practices.

---

Critical Issues

Finding 1: URL encoding applied twice results in double-encoded parameters

In HandlebarsMailService.cs:90, the fromMarketing parameter is URL-encoded before being assigned to the model:

FromMarketing = WebUtility.UrlEncode(fromMarketing),

Then in RegisterVerifyEmail.cs:22, it's concatenated into the URL without additional encoding:

!string.IsNullOrEmpty(FromMarketing) ? $"&fromMarketing={FromMarketing}" : string.Empty);

Problem: If fromMarketing contains the value "premium", it will be double-encoded:

• First encoding: "premium" → "premium" (no change, but still encoded)
• The issue arises if fromMarketing is already null or contains special characters

However, the real issue is encoding null:

• WebUtility.UrlEncode(null) returns null, not an empty string
• But the check !string.IsNullOrEmpty(FromMarketing) will still pass for an empty string

Recommendation: Only encode fromMarketing when constructing the URL, not when assigning to the model property:

Suggested fix

// In HandlebarsMailService.cs:90
FromMarketing = fromMarketing, // Don't encode here

// In RegisterVerifyEmail.cs:22
!string.IsNullOrEmpty(FromMarketing) ? $"&fromMarketing={WebUtility.UrlEncode(FromMarketing)}" : string.Empty);

Files: src/Core/Platform/Mail/HandlebarsMailService.cs:90, src/Core/Auth/Models/Mail/RegisterVerifyEmail.cs:22

---

Suggested Improvements

Finding 2: Missing XML documentation for new parameter

The fromMarketing parameter is added to several interfaces and implementations without XML documentation explaining its purpose, validation rules, or relationship to the feature flag.

Recommendation: Add XML documentation to clarify the parameter's purpose:

Suggested documentation

/// <summary>
/// Sends a registration verification email to the specified email address.
/// </summary>
/// <param name="email">The recipient email address</param>
/// <param name="token">The verification token</param>
/// <param name="fromMarketing">Optional marketing initiative identifier (e.g., "premium"). 
/// When provided, adds a query parameter to guide post-registration flow. 
/// Must be validated using MarketingInitiativeValidationAttribute.</param>
Task SendRegistrationVerificationEmailAsync(string email, string token, string? fromMarketing);

Files: src/Core/Platform/Mail/IMailService.cs:41, src/Core/Auth/UserFeatures/Registration/ISendVerificationEmailForRegistrationCommand.cs:6

---

Good Practices Observed

• Feature flag properly guards the new functionality in AccountsController.cs:112-115
• Comprehensive test coverage including feature flag enabled/disabled scenarios
• Input validation using custom MarketingInitiativeValidationAttribute
• Proper use of nullable reference types throughout
• Tests verify both the presence and absence of the marketing parameter

---

Additional Notes

Test Coverage

Codecov reports 40% patch coverage with 6 lines missing coverage in:

• RegisterVerifyEmail.cs (4 lines)
• HandlebarsMailService.cs (2 lines)

The missing coverage appears to be in the property assignments and URL construction logic. Consider adding integration tests that verify the actual email content and URL structure.

Security Considerations

• Input validation is properly implemented via MarketingInitiativeValidationAttribute
• URL encoding prevents injection attacks (though double-encoding issue noted above)
• Feature flag ensures controlled rollout
• No PII or sensitive data logged

---

PR Metadata Assessment

Title: ✅ Clear and specific

Objective: ✅ Well-explained with Jira reference and stacked PR context

Test Plan: ⚠️ Missing - No explicit test plan provided. The PR description should include manual testing steps or reference a test plan in the Jira task.

Suggestion: Add a test plan section describing how to manually verify:

1. Feature flag disabled → parameter not included in email link
2. Feature flag enabled + valid fromMarketing value → parameter included correctly
3. Feature flag enabled + invalid fromMarketing value → validation error
4. Feature flag enabled + null fromMarketing → parameter not included

[codecov]

Codecov Report

❌ Patch coverage is 40.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.99%. Comparing base (0422796) to head (d989ea1).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Core/Auth/Models/Mail/RegisterVerifyEmail.cs	0.00%	4 Missing ⚠️
src/Core/Platform/Mail/HandlebarsMailService.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6604      +/-   ##
==========================================
- Coverage   52.99%   52.99%   -0.01%     
==========================================
  Files        1910     1910              
  Lines       84843    84848       +5     
  Branches     7636     7638       +2     
==========================================
- Hits        44966    44965       -1     
- Misses      38125    38130       +5     
- Partials     1752     1753       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[dani-garcia]

Platform changes LGTM

The base branch was changed.