[PM-27575] Add support for loading Mailer templates from disk

[Hinton]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27575

📔 Objective

Adds support for overloading mail templates from disk.

⏰ 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

[claude]

Claude finished @Hinton's task —— View job

---

Code Review Complete

• Read and analyze changed files
• Review security implications
• Assess test coverage
• Check adherence to project standards
• Compile and post comprehensive review

---

Summary of Changes Since Last Review

This PR adds the capability to load mail templates from disk for self-hosted instances, allowing template customization without recompilation. The latest commits addressed path traversal vulnerabilities with case-insensitive security checks and comprehensive testing.

Recent changes:

• Commit 73e35d8: Improved path traversal protection using Path.GetFullPath() and Path.Combine()
• Commit 218b6d7: Added case-insensitive comparison to support Windows/macOS file systems and added corresponding tests

---

Critical Issues

1. Potential symlink bypass in path traversal protection (src/Core/Platform/Mailer/HandlebarMailRenderer.cs:97-106)

Severity: High (Security)

The current implementation resolves paths with Path.GetFullPath() but doesn't protect against symlink attacks. An attacker could create a symlink inside MailTemplateDirectory that points outside the directory.

// Current code
var diskPath = Path.GetFullPath(Path.Combine(_globalSettings.MailTemplateDirectory, template));
var baseDirectory = Path.GetFullPath(_globalSettings.MailTemplateDirectory);

if (!diskPath.StartsWith(baseDirectory + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) &&
    !diskPath.Equals(baseDirectory, StringComparison.OrdinalIgnoreCase))
{
    _logger.LogWarning("Template path traversal attempt detected: {Template}", template);
    return null;
}

Attack scenario:

1. Attacker gains write access to /mail-templates/ directory
2. Creates symlink: /mail-templates/evil.hbs -> /etc/passwd
3. Requests template evil.hbs
4. Path resolves to /mail-templates/evil.hbs (passes check)
5. File read follows symlink to /etc/passwd

Recommendation: Use Path.GetRealPath() (available in .NET 6+) or implement additional symlink detection:

var diskPath = Path.GetFullPath(Path.Combine(_globalSettings.MailTemplateDirectory, template));
var baseDirectory = Path.GetFullPath(_globalSettings.MailTemplateDirectory);

// Ensure the resolved path is within the configured directory
if (!diskPath.StartsWith(baseDirectory + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) &&
    !diskPath.Equals(baseDirectory, StringComparison.OrdinalIgnoreCase))
{
    _logger.LogWarning("Template path traversal attempt detected: {Template}", template);
    return null;
}

// Check for symlinks
var fileInfo = new FileInfo(diskPath);
if (fileInfo.Exists && fileInfo.Attributes.HasFlag(FileAttributes.ReparsePoint))
{
    _logger.LogWarning("Template is a symlink, which is not allowed: {Template}", template);
    return null;
}

2. Missing null check for MailTemplateDirectory (src/Core/Platform/Mailer/HandlebarMailRenderer.cs:97)

Severity: Medium (Reliability)

If _globalSettings.MailTemplateDirectory is null or empty, Path.Combine() will throw or behave unexpectedly.

Current code:

var diskPath = Path.GetFullPath(Path.Combine(_globalSettings.MailTemplateDirectory, template));

Recommendation:

if (string.IsNullOrWhiteSpace(_globalSettings.MailTemplateDirectory))
{
    return null;
}

var diskPath = Path.GetFullPath(Path.Combine(_globalSettings.MailTemplateDirectory, template));

3. File size limit missing (src/Core/Platform/Mailer/HandlebarMailRenderer.cs:110)

Severity: Medium (Security/Performance)

Reading arbitrary-sized files from disk without size validation could lead to:

• Memory exhaustion (DoS)
• Performance degradation
• Resource starvation

Current code:

if (File.Exists(diskPath))
{
    var fileContents = await File.ReadAllTextAsync(diskPath);
    return fileContents;
}

Recommendation:

const long MaxTemplateSize = 1024 * 1024; // 1MB

if (File.Exists(diskPath))
{
    var fileInfo = new FileInfo(diskPath);
    if (fileInfo.Length > MaxTemplateSize)
    {
        _logger.LogWarning("Template file too large: {Template} ({Size} bytes)", template, fileInfo.Length);
        return null;
    }
    
    var fileContents = await File.ReadAllTextAsync(diskPath);
    return fileContents;
}

---

Suggested Improvements

4. Test coverage gap: missing error handling tests

Severity: Low (Test Coverage)

The tests don't cover several important error scenarios:

• MailTemplateDirectory is null/empty
• File read throws UnauthorizedAccessException
• File read throws IOException
• File is locked by another process
• Large file handling

Recommendation: Add tests for these edge cases to improve robustness.

5. Race condition in file existence check (src/Core/Platform/Mailer/HandlebarMailRenderer.cs:108-112)

Severity: Low (Reliability)

Classic TOCTOU (Time-of-check Time-of-use) issue. File could be deleted between File.Exists() and File.ReadAllTextAsync().

Current code:

if (File.Exists(diskPath))
{
    var fileContents = await File.ReadAllTextAsync(diskPath);
    return fileContents;
}

Better approach:

try
{
    var fileContents = await File.ReadAllTextAsync(diskPath);
    return fileContents;
}
catch (FileNotFoundException)
{
    // File doesn't exist, return null
    return null;
}

This is already partially handled by the outer try-catch, but it would be cleaner to remove the File.Exists() check entirely and rely on exception handling.

6. Typo in test method name (test/Core.Test/Platform/Mailer/HandlebarMailRendererTests.cs:71)

Severity: Low (Code Quality)

public async Task ReadSourceFromDiskAsync_PrevenetsPathTraversal_WhenMaliciousPathProvided(string maliciousPath)

"Prevenets" should be "Prevents"

7. Inconsistent null-forgiving operator usage (src/Core/Platform/Mailer/HandlebarMailRenderer.cs:83)

Severity: Low (Code Consistency)

await using var s = assembly.GetManifestResourceStream(template)!;

The null-forgiving operator ! is used here, but earlier in the method (line 72), there's a check confirming the resource exists. This is correct, but a comment would make the intent clearer:

// Safe to use ! here because we verified the resource exists on line 72
await using var s = assembly.GetManifestResourceStream(template)!;

8. Consider extracting path validation to a separate method

Severity: Low (Maintainability)

The path validation logic in ReadSourceFromDiskAsync would benefit from being a separate, testable method:

private bool IsPathWithinDirectory(string filePath, string baseDirectory, out string normalizedPath)
{
    normalizedPath = Path.GetFullPath(filePath);
    var normalizedBase = Path.GetFullPath(baseDirectory);
    
    return normalizedPath.StartsWith(normalizedBase + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) ||
           normalizedPath.Equals(normalizedBase, StringComparison.OrdinalIgnoreCase);
}

This would make unit testing the path validation logic easier without using reflection.

---

Good Practices Observed

• Proper use of case-insensitive comparison for cross-platform compatibility
• Comprehensive unit tests including security-focused path traversal tests
• Proper exception handling with structured logging
• Thread-safe lazy initialization patterns maintained
• Feature gated by SelfHosted flag
• Graceful fallback to embedded resources when disk templates unavailable
• Test cleanup in finally blocks

---

Action Items for Author

1. HIGH PRIORITY: Add symlink protection to prevent directory traversal via symlinks (Issue Identity Server  #1)
2. HIGH PRIORITY: Add null/empty check for MailTemplateDirectory (Issue Password Requirements Validation #2)
3. MEDIUM PRIORITY: Implement file size validation to prevent DoS (Issue .NET Core Runtime #3)
4. MEDIUM PRIORITY: Add error handling tests for edge cases (Issue Licence #4)
5. LOW PRIORITY: Fix typo in test method name (Issue LDAP authentication? #6)
6. OPTIONAL: Consider refactoring path validation into a separate method for better testability (Issue Add a Gitter chat badge to README.md #8)

---

Coverage Note

Codecov reports 75.76% patch coverage with 8 lines missing coverage. The uncovered lines appear to be in error handling paths. Consider adding tests that trigger these paths to improve coverage to 90%+.

[Hinton]

We had to remove static from most functions because reading from disk relies on non static data.

[github-actions]

Checkmarx One – Scan Summary & Details – 9a91e508-d388-41f9-941c-c26dfc60ca36

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

[codecov]

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.88%. Comparing base (cfe818e) to head (218b6d7).

Files with missing lines	Patch %	Lines
src/Core/Platform/Mailer/HandlebarMailRenderer.cs	87.87%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6520      +/-   ##
==========================================
+ Coverage   51.86%   51.88%   +0.01%     
==========================================
  Files        1902     1902              
  Lines       84058    84090      +32     
  Branches     7501     7504       +3     
==========================================
+ Hits        43598    43629      +31     
- Misses      38766    38768       +2     
+ Partials     1694     1693       -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.