-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-27575] Add support for loading Mailer templates from disk #6520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a9853f8
73e35d8
218b6d7
40accc3
9e608b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,17 @@ | ||
| #nullable enable | ||
| using System.Collections.Concurrent; | ||
| using System.Reflection; | ||
| using Bit.Core.Settings; | ||
| using HandlebarsDotNet; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
||
| namespace Bit.Core.Platform.Mail.Mailer; | ||
| public class HandlebarMailRenderer : IMailRenderer | ||
| { | ||
| /// <summary> | ||
| /// Lazy-initialized Handlebars instance. Thread-safe and ensures initialization occurs only once. | ||
| /// </summary> | ||
| private readonly Lazy<Task<IHandlebars>> _handlebarsTask = new(InitializeHandlebarsAsync, LazyThreadSafetyMode.ExecutionAndPublication); | ||
| private readonly Lazy<Task<IHandlebars>> _handlebarsTask; | ||
|
|
||
| /// <summary> | ||
| /// Helper function that returns the handlebar instance. | ||
|
|
@@ -21,6 +23,17 @@ public class HandlebarMailRenderer : IMailRenderer | |
| /// </summary> | ||
| private readonly ConcurrentDictionary<string, Lazy<Task<HandlebarsTemplate<object, object>>>> _templateCache = new(); | ||
|
|
||
| private readonly ILogger<HandlebarMailRenderer> _logger; | ||
| private readonly GlobalSettings _globalSettings; | ||
|
|
||
| public HandlebarMailRenderer(ILogger<HandlebarMailRenderer> logger, GlobalSettings globalSettings) | ||
| { | ||
| _logger = logger; | ||
| _globalSettings = globalSettings; | ||
|
|
||
| _handlebarsTask = new Lazy<Task<IHandlebars>>(InitializeHandlebarsAsync, LazyThreadSafetyMode.ExecutionAndPublication); | ||
| } | ||
|
|
||
| public async Task<(string html, string txt)> RenderAsync(BaseMailView model) | ||
| { | ||
| var html = await CompileTemplateAsync(model, "html"); | ||
|
|
@@ -53,19 +66,59 @@ private async Task<HandlebarsTemplate<object, object>> CompileTemplateInternalAs | |
| return handlebars.Compile(source); | ||
| } | ||
|
|
||
| private static async Task<string> ReadSourceAsync(Assembly assembly, string template) | ||
| private async Task<string> ReadSourceAsync(Assembly assembly, string template) | ||
| { | ||
| if (assembly.GetManifestResourceNames().All(f => f != template)) | ||
| { | ||
| throw new FileNotFoundException("Template not found: " + template); | ||
| } | ||
|
|
||
| var diskSource = await ReadSourceFromDiskAsync(template); | ||
| if (!string.IsNullOrWhiteSpace(diskSource)) | ||
| { | ||
| return diskSource; | ||
| } | ||
|
|
||
| await using var s = assembly.GetManifestResourceStream(template)!; | ||
| using var sr = new StreamReader(s); | ||
| return await sr.ReadToEndAsync(); | ||
| } | ||
|
|
||
| private static async Task<IHandlebars> InitializeHandlebarsAsync() | ||
| private async Task<string?> ReadSourceFromDiskAsync(string template) | ||
| { | ||
| if (!_globalSettings.SelfHosted) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| var diskPath = Path.GetFullPath(Path.Combine(_globalSettings.MailTemplateDirectory, template)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ Finding 1: Null reference exception when DetailsWhen The
However, the check on line 89 only validates Suggested fix: if (!_globalSettings.SelfHosted || string.IsNullOrWhiteSpace(_globalSettings.MailTemplateDirectory))
{
return null;
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's caught by the catch right? |
||
| var baseDirectory = Path.GetFullPath(_globalSettings.MailTemplateDirectory); | ||
|
|
||
| // Ensure the resolved path is within the configured directory | ||
| if (!diskPath.StartsWith(baseDirectory + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) && | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The path validation uses Consider using |
||
| !diskPath.Equals(baseDirectory, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| _logger.LogWarning("Template path traversal attempt detected: {Template}", template); | ||
| return null; | ||
| } | ||
|
|
||
| if (File.Exists(diskPath)) | ||
| { | ||
| var fileContents = await File.ReadAllTextAsync(diskPath); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎨 Finding 3: Variable name The variable assignment on line 109 followed by immediate return on line 110 can be simplified to: return await File.ReadAllTextAsync(diskPath);This reduces unnecessary variable allocation and improves readability. |
||
| return fileContents; | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| _logger.LogError(e, "Failed to read mail template from disk: {TemplateName}", template); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private async Task<IHandlebars> InitializeHandlebarsAsync() | ||
| { | ||
| var handlebars = Handlebars.Create(); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had to remove static from most functions because reading from disk relies on non static data.