fix: Add path traversal protection and transactional batch generation

Document downloads now validate the file path stays within the storage dir.
Batch generation uses a transaction and cleans up files on failure. Added
background service to clean up orphaned PDF files daily.

Template ownership is now checked during document generation. PDF service
has a 30s timeout and better error handling. Pagination clamped to 1-100.

Author: jonmartin721

DocumentGenerator.API/Controllers/DocumentsController.cs

@@ -47,6 +47,10 @@ public async Task<ActionResult<PaginatedResult<DocumentDto>>> GetAll(
             [FromQuery] int page = 1,
             [FromQuery] int pageSize = 20)
         {
+            // Enforce pagination bounds
+            page = Math.Max(1, page);
+            pageSize = Math.Clamp(pageSize, 1, 100);
+
             var userId = this.GetUserId();
             return Ok(await _documentService.GetAllAsync(userId, page, pageSize));
         }

DocumentGenerator.API/Controllers/TemplatesController.cs

@@ -25,6 +25,10 @@ public async Task<ActionResult<PaginatedResult<TemplateDto>>> GetAll(
             [FromQuery] int page = 1,
             [FromQuery] int pageSize = 20)
         {
+            // Enforce pagination bounds
+            page = Math.Max(1, page);
+            pageSize = Math.Clamp(pageSize, 1, 100);
+
             var userId = this.GetUserId();
             return Ok(await _templateService.GetAllAsync(userId, page, pageSize));
         }

DocumentGenerator.API/Extensions/ServiceCollectionExtensions.cs

@@ -24,6 +24,9 @@ public static IServiceCollection AddInfrastructure(this IServiceCollection servi
             var jwtSettings = configuration.GetSection("JwtSettings").Get<JwtSettings>();
             services.Configure<JwtSettings>(configuration.GetSection("JwtSettings"));
 
+            // Storage Settings
+            services.Configure<StorageSettings>(configuration.GetSection("Storage"));
+
             // Authentication
             services.AddAuthentication(options =>
             {
@@ -40,7 +43,8 @@ public static IServiceCollection AddInfrastructure(this IServiceCollection servi
                     ValidateIssuerSigningKey = true,
                     ValidIssuer = jwtSettings?.Issuer,
                     ValidAudience = jwtSettings?.Audience,
-                    IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(jwtSettings?.Secret ?? throw new InvalidOperationException("JWT Secret is missing")))
+                    IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(jwtSettings?.Secret ?? throw new InvalidOperationException("JWT Secret is missing"))),
+                    ClockSkew = TimeSpan.FromMinutes(1)
                 };
             });
 
@@ -54,6 +58,7 @@ public static IServiceCollection AddInfrastructure(this IServiceCollection servi
             services.AddScoped<ITemplateService, TemplateService>();
             services.AddScoped<IDocumentService, DocumentService>();
             services.AddSingleton<IPdfService, PdfService>();
+            services.AddHostedService<OrphanCleanupService>();
 
             // AutoMapper
             services.AddAutoMapper(AppDomain.CurrentDomain.GetAssemblies());

DocumentGenerator.API/appsettings.json

@@ -15,6 +15,9 @@
     "ExpiryMinutes": 60,
     "RefreshTokenExpiryDays": 7
   },
+  "Storage": {
+    "DocumentsPath": "GeneratedDocuments"
+  },
   "AllowedOrigins": [
     "http://localhost:5173"
   ]

DocumentGenerator.Core/DTOs/DocumentDtos.cs

@@ -31,6 +31,7 @@ public class BatchGenerationRequestDto
 
         [Required]
         [MinLength(1, ErrorMessage = "At least one data item is required")]
+        [MaxLength(100, ErrorMessage = "Batch size cannot exceed 100 items")]
         public List<object> DataItems { get; set; } = new();
     }
 

DocumentGenerator.Core/Settings/StorageSettings.cs

@@ -0,0 +1,7 @@
+namespace DocumentGenerator.Core.Settings
+{
+    public class StorageSettings
+    {
+        public string DocumentsPath { get; set; } = "GeneratedDocuments";
+    }
+}

DocumentGenerator.Core/Validators/TemplateValidators.cs

@@ -13,17 +13,18 @@ public CreateTemplateDtoValidator()
 
             RuleFor(x => x.Content)
                 .NotEmpty().WithMessage("Template content is required")
+                .MaximumLength(1_000_000).WithMessage("Template content exceeds maximum size (1MB)")
                 .Must(BeValidHandlebars).WithMessage("Invalid Handlebars syntax");
         }
 
-        private bool BeValidHandlebars(string content)
+        private static bool BeValidHandlebars(string content)
         {
             try
             {
                 Handlebars.Compile(content);
                 return true;
             }
-            catch
+            catch (HandlebarsException)
             {
                 return false;
             }
@@ -35,19 +36,23 @@ public class UpdateTemplateDtoValidator : AbstractValidator<UpdateTemplateDto>
         public UpdateTemplateDtoValidator()
         {
             RuleFor(x => x.Content)
-                .Must(BeValidHandlebars)
+                .MaximumLength(1_000_000).WithMessage("Template content exceeds maximum size (1MB)")
+                .When(x => !string.IsNullOrEmpty(x.Content));
+
+            RuleFor(x => x.Content)
+                .Must(BeValidHandlebars!)
                 .When(x => !string.IsNullOrEmpty(x.Content))
                 .WithMessage("Invalid Handlebars syntax");
         }
 
-        private bool BeValidHandlebars(string content)
+        private static bool BeValidHandlebars(string content)
         {
             try
             {
                 Handlebars.Compile(content);
                 return true;
             }
-            catch
+            catch (HandlebarsException)
             {
                 return false;
             }

DocumentGenerator.Infrastructure/DocumentGenerator.Infrastructure.csproj

@@ -10,6 +10,7 @@
     <PackageReference Include="Handlebars.Net" Version="2.1.6" />
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.11" />
     <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="10.0.0" />
+    <PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="9.0.0" />
     <PackageReference Include="Microsoft.Extensions.Options" Version="10.0.0" />
     <PackageReference Include="PuppeteerSharp" Version="20.2.4" />
     <PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="8.15.0" />

DocumentGenerator.Infrastructure/Services/DocumentService.cs

@@ -2,10 +2,12 @@
 using DocumentGenerator.Core.DTOs;
 using DocumentGenerator.Core.Entities;
 using DocumentGenerator.Core.Interfaces;
+using DocumentGenerator.Core.Settings;
 using DocumentGenerator.Infrastructure.Data;
 using HandlebarsDotNet;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 using System.Text.Json;
 
 namespace DocumentGenerator.Infrastructure.Services
@@ -22,13 +24,19 @@ public DocumentService(
             ApplicationDbContext context,
             IPdfService pdfService,
             IMapper mapper,
-            ILogger<DocumentService> logger)
+            ILogger<DocumentService> logger,
+            IOptions<StorageSettings> storageSettings)
         {
             _context = context;
             _pdfService = pdfService;
             _mapper = mapper;
             _logger = logger;
-            _storagePath = Path.Combine(Directory.GetCurrentDirectory(), "GeneratedDocuments");
+
+            var documentsPath = storageSettings.Value.DocumentsPath;
+            _storagePath = Path.IsPathRooted(documentsPath)
+                ? documentsPath
+                : Path.Combine(Directory.GetCurrentDirectory(), documentsPath);
+
             if (!Directory.Exists(_storagePath))
             {
                 Directory.CreateDirectory(_storagePath);
@@ -37,10 +45,11 @@ public DocumentService(
 
         public async Task<DocumentDto> GenerateDocumentAsync(GenerationRequestDto request, Guid userId)
         {
-            var template = await _context.Templates.FindAsync(request.TemplateId);
+            var template = await _context.Templates
+                .FirstOrDefaultAsync(t => t.Id == request.TemplateId && t.CreatedByUserId == userId);
             if (template == null)
             {
-                _logger.LogWarning("Template {TemplateId} not found for document generation", request.TemplateId);
+                _logger.LogWarning("Template {TemplateId} not found or not owned by user {UserId}", request.TemplateId, userId);
                 throw new KeyNotFoundException("Template not found");
             }
 
@@ -87,29 +96,41 @@ public async Task<BatchGenerationResultDto> GenerateDocumentBatchAsync(BatchGene
                 TotalRequested = request.DataItems.Count
             };
 
-            var template = await _context.Templates.FindAsync(request.TemplateId);
+            var template = await _context.Templates
+                .FirstOrDefaultAsync(t => t.Id == request.TemplateId && t.CreatedByUserId == userId);
             if (template == null)
             {
-                _logger.LogWarning("Template {TemplateId} not found for batch document generation", request.TemplateId);
+                _logger.LogWarning("Template {TemplateId} not found or not owned by user {UserId} for batch generation", request.TemplateId, userId);
                 throw new KeyNotFoundException("Template not found");
             }
 
             _logger.LogInformation("Starting batch generation of {Count} documents from template {TemplateId} for user {UserId}",
                 request.DataItems.Count, request.TemplateId, userId);
 
             var compiledTemplate = Handlebars.Compile(template.Content);
+            var generatedFiles = new List<string>();
 
-            for (int i = 0; i < request.DataItems.Count; i++)
+            await using var transaction = await _context.Database.BeginTransactionAsync();
+            try
             {
-                try
+                for (int i = 0; i < request.DataItems.Count; i++)
                 {
                     var data = request.DataItems[i];
                     var htmlContent = compiledTemplate(data);
                     var pdfBytes = await _pdfService.GeneratePdfAsync(htmlContent);
 
                     var fileName = $"doc-{Guid.NewGuid()}.pdf";
                     var filePath = Path.Combine(_storagePath, fileName);
-                    await File.WriteAllBytesAsync(filePath, pdfBytes);
+
+                    try
+                    {
+                        await File.WriteAllBytesAsync(filePath, pdfBytes);
+                        generatedFiles.Add(filePath);
+                    }
+                    catch (IOException ex)
+                    {
+                        throw new InvalidOperationException($"Failed to save document file at index {i}", ex);
+                    }
 
                     var document = new Document
                     {
@@ -130,24 +151,27 @@ public async Task<BatchGenerationResultDto> GenerateDocumentBatchAsync(BatchGene
                     result.Documents.Add(dto);
                     result.SuccessCount++;
                 }
-                catch (Exception ex)
-                {
-                    _logger.LogWarning(ex, "Failed to generate document at index {Index} in batch", i);
-                    result.Errors.Add(new BatchGenerationError
-                    {
-                        Index = i,
-                        Message = ex.Message
-                    });
-                    result.FailureCount++;
-                }
+
+                await _context.SaveChangesAsync();
+                await transaction.CommitAsync();
+
+                _logger.LogInformation("Batch generation completed successfully: {Count} documents generated", result.SuccessCount);
+                return result;
             }
+            catch (Exception ex)
+            {
+                _logger.LogError(ex, "Batch generation failed, rolling back {Count} files", generatedFiles.Count);
 
-            await _context.SaveChangesAsync();
+                await transaction.RollbackAsync();
 
-            _logger.LogInformation("Batch generation completed: {Success} succeeded, {Failed} failed",
-                result.SuccessCount, result.FailureCount);
+                // Clean up any files that were written
+                foreach (var file in generatedFiles)
+                {
+                    DeleteFileIfExists(file);
+                }
 
-            return result;
+                throw;
+            }
         }
 
         public async Task<DocumentDto?> GetByIdAsync(Guid id, Guid userId)
@@ -195,11 +219,26 @@ public async Task<PaginatedResult<DocumentDto>> GetAllAsync(Guid userId, int pag
             var document = await _context.Documents.FindAsync(id);
             if (document == null || document.UserId != userId) return null;
 
+            // Validate path is within storage directory (path traversal protection)
+            if (!IsPathSafe(document.StoragePath))
+            {
+                _logger.LogWarning("Attempted path traversal detected for document {DocumentId}", id);
+                throw new UnauthorizedAccessException("Invalid document path");
+            }
+
             if (!File.Exists(document.StoragePath))
                 throw new FileNotFoundException("Document file not found on server");
 
-            var bytes = await File.ReadAllBytesAsync(document.StoragePath);
-            return (bytes, Path.GetFileName(document.StoragePath));
+            try
+            {
+                var bytes = await File.ReadAllBytesAsync(document.StoragePath);
+                return (bytes, Path.GetFileName(document.StoragePath));
+            }
+            catch (IOException ex)
+            {
+                _logger.LogError(ex, "Failed to read document file {DocumentId}", id);
+                throw new InvalidOperationException("Failed to read document file", ex);
+            }
         }
 
         public async Task<bool> DeleteAsync(Guid id, Guid userId)
@@ -211,11 +250,15 @@ public async Task<bool> DeleteAsync(Guid id, Guid userId)
                 return false;
             }
 
-            DeleteFileIfExists(document.StoragePath);
+            var storagePath = document.StoragePath;
 
+            // Delete from database first to prevent race conditions
             _context.Documents.Remove(document);
             await _context.SaveChangesAsync();
 
+            // Then delete the file (after DB commit)
+            DeleteFileIfExists(storagePath);
+
             _logger.LogInformation("Document {DocumentId} deleted by user {UserId}", id, userId);
             return true;
         }
@@ -226,14 +269,22 @@ public async Task DeleteByTemplateIdAsync(Guid templateId)
                 .Where(d => d.TemplateId == templateId)
                 .ToListAsync();
 
+            // Collect file paths before removing from context
+            var filePaths = documents.Select(d => d.StoragePath).ToList();
+
             foreach (var doc in documents)
             {
-                DeleteFileIfExists(doc.StoragePath);
                 _context.Documents.Remove(doc);
             }
 
             await _context.SaveChangesAsync();
 
+            // Delete files after DB commit
+            foreach (var path in filePaths)
+            {
+                DeleteFileIfExists(path);
+            }
+
             _logger.LogInformation("Deleted {Count} documents for template {TemplateId}", documents.Count, templateId);
         }
 
@@ -251,5 +302,19 @@ private void DeleteFileIfExists(string path)
                 }
             }
         }
+
+        private bool IsPathSafe(string filePath)
+        {
+            try
+            {
+                var fullPath = Path.GetFullPath(filePath);
+                var storagePath = Path.GetFullPath(_storagePath);
+                return fullPath.StartsWith(storagePath, StringComparison.OrdinalIgnoreCase);
+            }
+            catch
+            {
+                return false;
+            }
+        }
     }
 }