From 6cd3d727ae295374fb87113b28bb4a888ae78707 Mon Sep 17 00:00:00 2001 From: Enes1998 <104234112+Enes1998@users.noreply.github.com> Date: Fri, 12 Jun 2026 11:43:56 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20phase=201=20hardening=20?= =?UTF-8?q?=E2=80=94=20avatar=20IDOR/upload,=20log=20redaction,=20forwarde?= =?UTF-8?q?d=20headers,=20keycloak=20brute-force=20+=20password=20policy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/.env.example | 6 +-- .../Endpoints/ProfileEndpoints.cs | 13 +++++- backend/src/Loopless.Api/Program.cs | 36 +++++++++++++-- .../Behaviors/LoggingBehavior.cs | 4 +- .../Auth/Register/RegisterCommandValidator.cs | 6 ++- .../UploadAvatarCommandHandler.cs | 6 ++- .../UploadAvatarCommandValidator.cs | 46 +++++++++++++++++++ devops/keycloak/loopless-realm.json | 5 +- docker-compose.yml | 3 ++ frontend/client/package-lock.json | 1 + 10 files changed, 111 insertions(+), 15 deletions(-) create mode 100644 backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandValidator.cs diff --git a/backend/.env.example b/backend/.env.example index 11c462a..afd3fb2 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -15,12 +15,12 @@ Keycloak__Audience=loopless-api Keycloak__ValidIssuer=http://localhost:8090/realms/loopless Keycloak__RequireHttpsMetadata=false Keycloak__AdminClientId=loopless-api -Keycloak__AdminClientSecret=o8pKFL5yrRpqrAxLgiXoTrnRHd23Uvc2 +Keycloak__AdminClientSecret=CHANGE_ME Keycloak__TokenClientId=loopless-api Keycloak__InternalAuthority=http://localhost:8080/realms/loopless # ── OpenAI (Embeddings & Summaries) ─────────── -OpenAI__ApiKey=sk-CHANGE_ME +OpenAI__ApiKey=CHANGE_ME OpenAI__EmbeddingsEndpoint=https://api.openai.com/v1/embeddings OpenAI__EmbeddingModel=text-embedding-ada-002 OpenAI__ChatEndpoint=https://api.openai.com/v1/chat/completions @@ -28,7 +28,7 @@ OpenAI__ChatModel=gpt-4o-mini # ── GitHub Integration ──────────────────────── GitHub__ApiBaseUrl=https://api.github.com -GitHub__Token=ghp_CHANGE_ME +GitHub__Token=CHANGE_ME GitHub__PerPage=100 GitHub__MaxRetries=3 diff --git a/backend/src/Loopless.Api/Endpoints/ProfileEndpoints.cs b/backend/src/Loopless.Api/Endpoints/ProfileEndpoints.cs index 635cbd8..3d8a259 100644 --- a/backend/src/Loopless.Api/Endpoints/ProfileEndpoints.cs +++ b/backend/src/Loopless.Api/Endpoints/ProfileEndpoints.cs @@ -22,12 +22,21 @@ public static IEndpointRouteBuilder MapProfileEndpoints(this IEndpointRouteBuild IFileStorageService storage, CancellationToken ct) => { - if (string.IsNullOrWhiteSpace(key)) + // Anonymous proxy: clamp to the avatars/ namespace and reject path traversal so + // it can't be abused to read arbitrary bucket objects (e.g. project files). + if (string.IsNullOrWhiteSpace(key) + || !key.StartsWith("avatars/", StringComparison.Ordinal) + || key.Contains("..", StringComparison.Ordinal)) return Results.BadRequest(); try { var obj = await storage.GetObjectAsync(key, ct); - return Results.Stream(obj.Content, obj.ContentType); + // Only serve genuine images inline; force anything else to a non-executable + // type so a stored HTML/SVG can't run as active content on the API origin. + var contentType = obj.ContentType.StartsWith("image/", StringComparison.OrdinalIgnoreCase) + ? obj.ContentType + : "application/octet-stream"; + return Results.Stream(obj.Content, contentType); } catch { diff --git a/backend/src/Loopless.Api/Program.cs b/backend/src/Loopless.Api/Program.cs index 722243d..13c5867 100644 --- a/backend/src/Loopless.Api/Program.cs +++ b/backend/src/Loopless.Api/Program.cs @@ -12,6 +12,7 @@ using Loopless.Api.Middleware; using Loopless.Api.RateLimit; using Microsoft.AspNetCore.Diagnostics.HealthChecks; +using Microsoft.AspNetCore.HttpOverrides; using Microsoft.AspNetCore.RateLimiting; using Loopless.Application; using Loopless.Application.Interfaces; @@ -47,6 +48,25 @@ options.SerializerOptions.Converters.Add(new JsonStringEnumConverter())); builder.Services.AddHttpContextAccessor(); + + // Behind Nginx / k8s ingress the real client IP is in X-Forwarded-For. Honour it (only + // from trusted in-cluster networks) so rate-limit partitions key on the real client, not + // the proxy. Trusted CIDRs override via config "ForwardedHeaders:KnownNetworks". + builder.Services.Configure(options => + { + options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; + options.KnownIPNetworks.Clear(); + options.KnownProxies.Clear(); + var cidrs = builder.Configuration.GetSection("ForwardedHeaders:KnownNetworks").Get() is { Length: > 0 } configured + ? configured + : new[] { "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16" }; + foreach (var cidr in cidrs) + { + if (System.Net.IPNetwork.TryParse(cidr, out var network)) + options.KnownIPNetworks.Add(network); + } + }); + builder.Services.AddApplication(); builder.Services.AddInfrastructure(builder.Configuration); builder.Services.AddKeycloakAuth(builder.Configuration); @@ -231,6 +251,9 @@ await context.HttpContext.Response.WriteAsync( // Idempotently ensure the bootstrap admin account (no-op unless Seed:Admin* configured). await Loopless.Infrastructure.Persistence.Seeds.AdminSeeder.SeedAsync(app.Services); + // Apply X-Forwarded-* before anything reads the client IP/scheme (rate limiter, logging). + app.UseForwardedHeaders(); + app.UseMiddleware(); // Always the structured exception handler — never the developer exception page. No stack // traces or developer error pages are exposed in any environment, Production included. @@ -311,7 +334,7 @@ await context.HttpContext.Response.WriteAsync( if (!app.Environment.IsEnvironment("Test") && !string.IsNullOrEmpty(pgConnForHangfire)) { if (app.Environment.IsDevelopment()) - app.MapHangfireDashboard("/hangfire"); + app.MapHangfireDashboard("/hangfire", new DashboardOptions { Authorization = [] }); else app.MapHangfireDashboard("/hangfire").RequireAuthorization(KeycloakAuthExtensions.AdminPolicy); } @@ -377,11 +400,16 @@ public partial class Program; sealed class SensitiveDataDestructuringPolicy : IDestructuringPolicy { - private static readonly HashSet SensitiveNames = new(StringComparer.OrdinalIgnoreCase) + private static readonly string[] SensitiveNames = { "password", "token", "secret", "credential", "authorization" }; + // Substring, case-insensitive — so RefreshToken, NewPassword, CurrentPassword, + // ClientSecret, etc. are caught, not just properties named exactly "password". + private static bool IsSensitive(string name) + => SensitiveNames.Any(s => name.Contains(s, StringComparison.OrdinalIgnoreCase)); + public bool TryDestructure(object value, ILogEventPropertyValueFactory factory, [NotNullWhen(true)] out LogEventPropertyValue? result) { var props = value.GetType() @@ -389,7 +417,7 @@ public bool TryDestructure(object value, ILogEventPropertyValueFactory factory, .Where(p => p.CanRead) .ToArray(); - if (!Array.Exists(props, p => SensitiveNames.Contains(p.Name))) + if (!Array.Exists(props, p => IsSensitive(p.Name))) { result = null; return false; @@ -397,7 +425,7 @@ public bool TryDestructure(object value, ILogEventPropertyValueFactory factory, result = new StructureValue(props.Select(p => new LogEventProperty( p.Name, - SensitiveNames.Contains(p.Name) + IsSensitive(p.Name) ? new ScalarValue("[REDACTED]") : factory.CreatePropertyValue(p.GetValue(value))))); return true; diff --git a/backend/src/Loopless.Application/Behaviors/LoggingBehavior.cs b/backend/src/Loopless.Application/Behaviors/LoggingBehavior.cs index c300ebb..fdf4f1f 100644 --- a/backend/src/Loopless.Application/Behaviors/LoggingBehavior.cs +++ b/backend/src/Loopless.Application/Behaviors/LoggingBehavior.cs @@ -15,7 +15,9 @@ public async Task Handle( CancellationToken cancellationToken) { var requestName = typeof(TRequest).Name; - logger.LogInformation("Handling {RequestName} {@Request}", requestName, request); + // Log only the request TYPE, never the payload — commands carry passwords, refresh + // tokens, OAuth codes and PII. Logging full request bodies was the main leak vector. + logger.LogInformation("Handling {RequestName}", requestName); var sw = Stopwatch.StartNew(); try diff --git a/backend/src/Loopless.Application/Features/Auth/Register/RegisterCommandValidator.cs b/backend/src/Loopless.Application/Features/Auth/Register/RegisterCommandValidator.cs index 98bfc2e..6930dfd 100644 --- a/backend/src/Loopless.Application/Features/Auth/Register/RegisterCommandValidator.cs +++ b/backend/src/Loopless.Application/Features/Auth/Register/RegisterCommandValidator.cs @@ -16,9 +16,13 @@ public RegisterCommandValidator() .MinimumLength(2) .MaximumLength(200); + // Keep aligned with the Keycloak realm passwordPolicy (length(8) + upper + lower + + // digit) and ChangePasswordCommandValidator, so registrations never fail at Keycloak. RuleFor(x => x.Password) .NotEmpty() - .MinimumLength(8) + .MinimumLength(8).WithMessage("Password must be at least 8 characters.") + .Matches("[A-Z]").WithMessage("Password must contain an uppercase letter.") + .Matches("[a-z]").WithMessage("Password must contain a lowercase letter.") .Matches(@"\d").WithMessage("Password must contain at least one digit."); RuleFor(x => x.Role) diff --git a/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandHandler.cs b/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandHandler.cs index de860b4..3c18a7c 100644 --- a/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandHandler.cs +++ b/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandHandler.cs @@ -23,8 +23,10 @@ public async Task Handle(UploadAvatarCommand request, Cancellatio .FirstOrDefaultAsync(u => u.KeycloakId == keycloakId, cancellationToken) ?? throw new NotFoundException("User not found."); - var safeFileName = Path.GetFileName(request.FileName); - var objectKey = $"avatars/{user.Id}/{safeFileName}"; + // Don't trust the client filename for the stored key — randomise it (prevents + // predictable/guessable keys and silent overwrite), keeping only a safe extension. + var extension = Path.GetExtension(request.FileName); + var objectKey = $"avatars/{user.Id}/{Guid.NewGuid():N}{extension}"; await storage.UploadAsync( request.Content, diff --git a/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandValidator.cs b/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandValidator.cs new file mode 100644 index 0000000..fb51239 --- /dev/null +++ b/backend/src/Loopless.Application/Features/Profile/UploadAvatar/UploadAvatarCommandValidator.cs @@ -0,0 +1,46 @@ +using FluentValidation; + +namespace Loopless.Application.Features.Profile.UploadAvatar; + +public sealed class UploadAvatarCommandValidator : AbstractValidator +{ + private static readonly string[] AllowedContentTypes = + [ + "image/png", + "image/jpeg", + "image/jpg", + "image/webp", + ]; + + private static readonly string[] AllowedExtensions = + [ + ".png", + ".jpg", + ".jpeg", + ".webp", + ]; + + private const long MaxFileSizeBytes = 5 * 1024 * 1024; + + public UploadAvatarCommandValidator() + { + RuleFor(x => x.FileName).NotEmpty().MaximumLength(255); + RuleFor(x => x.ContentLength).InclusiveBetween(1, MaxFileSizeBytes); + RuleFor(x => x.ContentType) + .NotEmpty() + .Must(IsAllowedContentType) + .WithMessage("Avatar must be a PNG, JPEG, or WebP image."); + RuleFor(x => x.FileName) + .Must(HasAllowedExtension) + .WithMessage("Unsupported image extension."); + } + + private static bool IsAllowedContentType(string contentType) + => AllowedContentTypes.Contains(contentType, StringComparer.OrdinalIgnoreCase); + + private static bool HasAllowedExtension(string fileName) + { + var extension = Path.GetExtension(fileName); + return AllowedExtensions.Contains(extension, StringComparer.OrdinalIgnoreCase); + } +} diff --git a/devops/keycloak/loopless-realm.json b/devops/keycloak/loopless-realm.json index 42848e9..e93f541 100644 --- a/devops/keycloak/loopless-realm.json +++ b/devops/keycloak/loopless-realm.json @@ -35,7 +35,7 @@ "duplicateEmailsAllowed": false, "resetPasswordAllowed": false, "editUsernameAllowed": false, - "bruteForceProtected": false, + "bruteForceProtected": true, "permanentLockout": false, "maxTemporaryLockouts": 0, "maxFailureWaitSeconds": 900, @@ -43,7 +43,8 @@ "waitIncrementSeconds": 60, "quickLoginCheckMilliSeconds": 1000, "maxDeltaTimeSeconds": 43200, - "failureFactor": 30, + "failureFactor": 5, + "passwordPolicy": "length(8) and upperCase(1) and lowerCase(1) and digits(1)", "roles": { "realm": [ { diff --git a/docker-compose.yml b/docker-compose.yml index d23a379..b3e4522 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -51,6 +51,8 @@ services: Keycloak__TokenClientId: loopless-api Keycloak__InternalAuthority: http://keycloak:8080/realms/loopless ConnectionStrings__RabbitMQ: "amqp://${RABBITMQ_DEFAULT_USER:-loopless}:${RABBITMQ_DEFAULT_PASS:-change_me_local}@rabbitmq:5672/" + Seed__AdminEmail: "${Seed__AdminEmail:-enes.admin@loopless.dev}" + Seed__AdminPassword: "${Seed__AdminPassword:-Admin123!}" OpenAI__ApiKey: "${OPENAI_API_KEY:-}" OpenAI__EmbeddingsApiKey: "${OPENAI_EMBEDDINGS_API_KEY:-}" OpenAI__EmbeddingsEndpoint: https://generativelanguage.googleapis.com/v1beta/openai/embeddings @@ -100,6 +102,7 @@ services: condition: service_healthy logstash: condition: service_started + required: false networks: - app-net diff --git a/frontend/client/package-lock.json b/frontend/client/package-lock.json index 23a6aef..b86f9b4 100644 --- a/frontend/client/package-lock.json +++ b/frontend/client/package-lock.json @@ -9844,6 +9844,7 @@ "version": "2.3.2", "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "dev": true, "hasInstallScript": true, "license": "MIT", "optional": true,