From fd41983b9fd1b994b7a7f32c893d143c3d0e28a1 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 27 Aug 2026 18:49:34 -0700 Subject: [PATCH 1/2] fix(auth): build CAS callbacks from a configured canonical origin CAS login, ticket validation and logout derived their service URL from HttpHelper.GetRootURL(), which reads the request Host, and AllowedHosts was "*" in every environment. A Host header that got past the proxies could therefore poison a CAS callback. - Application:PublicBaseUrl per environment, validated on start so a deployed environment fails fast rather than falling back to the request - AllowedHosts narrowed to the real TEST/PROD hostnames plus localhost - GetRootURL() returns the canonical origin when configured, so the sitemap and emulation links stop being request-derived too - Login's /api guard normalizes the "~/" app-relative form, strips the PathBase and matches whole path segments, so an API ReturnUrl gets a 401 instead of a CAS HTML redirect under the deployed /2 sub-app, while /apiary stays a normal page - Retire EmailSettings:BaseUrl, which held the same public origin under an email-specific name. Email links, the health-check collector and CAS now read one setting, so the two cannot drift NormalizeAppRelativeUrl, IsApiPath and StripPathBase are shared with the dynamic login screen stacked above, so they live here at the bottom. --- test/Classes/HomeControllerCasUrlTests.cs | 205 ++++++++++++++++++ test/Classes/PublicUrlServiceTests.cs | 185 ++++++++++++++++ .../EmailNotificationTest.cs | 15 +- .../ControllerServiceIntegrationTest.cs | 8 +- .../ScheduleEditServiceRollbackTest.cs | 8 +- .../ScheduleEditServiceTest.cs | 8 +- test/ClinicalScheduler/TestDataBuilder.cs | 2 +- .../TestableScheduleEditService.cs | 5 +- test/Effort/EffortIntegrationTestBase.cs | 2 +- test/Effort/VerificationServiceTests.cs | 55 +---- .../Services/ScheduleEditService.cs | 15 +- .../Effort/Services/VerificationService.cs | 49 +---- .../HealthChecks/HealthCheckExtensions.cs | 7 +- web/Classes/HttpHelper.cs | 36 +-- web/Classes/PublicUrlService.cs | 174 +++++++++++++++ web/Controllers/HomeController.cs | 69 +++++- web/Program.cs | 26 +-- web/Services/EmailService.cs | 6 - web/appsettings.Production.json | 11 +- web/appsettings.Test.json | 9 +- 20 files changed, 714 insertions(+), 181 deletions(-) create mode 100644 test/Classes/HomeControllerCasUrlTests.cs create mode 100644 test/Classes/PublicUrlServiceTests.cs create mode 100644 web/Classes/PublicUrlService.cs diff --git a/test/Classes/HomeControllerCasUrlTests.cs b/test/Classes/HomeControllerCasUrlTests.cs new file mode 100644 index 000000000..c419d7baa --- /dev/null +++ b/test/Classes/HomeControllerCasUrlTests.cs @@ -0,0 +1,205 @@ +using System.Net; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Options; +using NSubstitute; +using Viper.Classes; +using Viper.Classes.SQLContext; +using Viper.Controllers; +using Web.Authorization; + +namespace Viper.test.Classes; + +/// +/// CAS service callbacks must be built from the configured canonical origin, never from the +/// request Host. Login covers the shared BuildRedirectUri helper that CasLogin's ticket +/// validation also uses. +/// +public class HomeControllerCasUrlTests +{ + private const string CasBaseUrl = "https://ssodev.ucdavis.edu/cas/"; + private const string PublicBaseUrl = "https://secure-test.vetmed.ucdavis.edu/2"; + private const string ForgedHost = "attacker.example"; + + [Fact] + public void Login_BuildsServiceFromConfiguredOrigin_NotHostHeader() + { + var controller = CreateController(ForgedHost, pathBase: "/2"); + + var result = Assert.IsType(controller.Login()); + + Assert.DoesNotContain(ForgedHost, result.Url, StringComparison.OrdinalIgnoreCase); + Assert.StartsWith($"{PublicBaseUrl}/CasLogin?", ServiceParameter(result.Url), StringComparison.Ordinal); + } + + [Fact] + public void Login_DefaultReturnUrl_PreservesPathBase() + { + var controller = CreateController(ForgedHost, pathBase: "/2"); + + var result = Assert.IsType(controller.Login()); + + // ReturnUrl is encoded inside the service value, which is then encoded again for CAS, + // so one decode leaves the inner encoding intact. + Assert.Equal($"{PublicBaseUrl}/CasLogin?ReturnUrl={WebUtility.UrlEncode("/2")}", ServiceParameter(result.Url)); + } + + [Fact] + public void Login_NoPathBase_DefaultsToEmptyReturnUrl() + { + var controller = CreateController("localhost:7157", pathBase: string.Empty); + + var result = Assert.IsType(controller.Login()); + + Assert.Equal($"{PublicBaseUrl}/CasLogin?ReturnUrl=", ServiceParameter(result.Url)); + } + + [Fact] + public void Login_ExplicitReturnUrl_IsPreserved() + { + var controller = CreateController(ForgedHost, pathBase: "/2"); + + var result = Assert.IsType(controller.Login("/2/Students/StudentClassYear")); + + Assert.Equal( + $"{PublicBaseUrl}/CasLogin?ReturnUrl={WebUtility.UrlEncode("/2/Students/StudentClassYear")}", + ServiceParameter(result.Url)); + } + + [Fact] + public void Login_ApiReturnUrlUnderPathBase_ReturnsUnauthorized() + { + // The SPAs send ReturnUrl already prefixed with the deployed PathBase, so without + // stripping it the API guard never fired on TEST/PROD and an API caller got a CAS + // HTML redirect instead of a 401. + var controller = CreateController("secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + Assert.IsType(controller.Login("/2/api/students/dvm")); + } + + [Fact] + public void Login_ApiReturnUrlWithoutPathBase_ReturnsUnauthorized() + { + var controller = CreateController("localhost:7157", pathBase: string.Empty); + + Assert.IsType(controller.Login("/api/students/dvm")); + } + + [Fact] + public void Login_PathLookingLikeApi_IsNotTreatedAsApi() + { + // "/apiary" shares a prefix with "/api" but is not under it. + var controller = CreateController("secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + var result = Assert.IsType(controller.Login("/2/apiary/hives")); + + Assert.Equal( + $"{PublicBaseUrl}/CasLogin?ReturnUrl={WebUtility.UrlEncode("/2/apiary/hives")}", + ServiceParameter(result.Url)); + } + + [Fact] + public void Login_ApiReturnUrl_WhenBasePathPrefixesIt_ReturnsUnauthorized() + { + // A base that is a character prefix of "/api" must not be stripped off "/api/...", + // or the guard chops the URL into something it no longer recognizes as an API path. + var controller = CreateController("secure-test.vetmed.ucdavis.edu", pathBase: "/a"); + + Assert.IsType(controller.Login("/api/students/dvm")); + } + + [Fact] + public void Login_BareApiReturnUrl_ReturnsUnauthorized() + { + var controller = CreateController("secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + Assert.IsType(controller.Login("/2/api")); + } + + [Theory] + [InlineData("~/api/students/dvm")] + [InlineData("~/2/api/students/dvm")] + [InlineData("~/API/students/dvm")] + public void Login_AppRelativeApiReturnUrl_ReturnsUnauthorized(string returnUrl) + { + // "~/api/..." resolves against the PathBase and reaches the same endpoint as + // "/api/...", so it must not slip past the guard. + var controller = CreateController("secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + Assert.IsType(controller.Login(returnUrl)); + } + + [Fact] + public void Login_AppRelativePathLookingLikeApi_IsNotTreatedAsApi() + { + var controller = CreateController("secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + var result = Assert.IsType(controller.Login("~/apiary/hives")); + + // The "~" is normalized off before the URL is handed to CAS, which does not understand it. + Assert.Equal( + $"{PublicBaseUrl}/CasLogin?ReturnUrl={WebUtility.UrlEncode("/apiary/hives")}", + ServiceParameter(result.Url)); + } + + [Fact] + public async Task Logout_BuildsServiceFromConfiguredOrigin_NotHostHeader() + { + var controller = CreateController(ForgedHost, pathBase: "/2"); + + var result = Assert.IsType(await controller.Logout()); + + Assert.DoesNotContain(ForgedHost, result.Url, StringComparison.OrdinalIgnoreCase); + Assert.Equal($"{CasBaseUrl}logout?service={WebUtility.UrlEncode(PublicBaseUrl)}", result.Url); + } + + /// + /// Pulls the decoded CAS service parameter out of the redirect so assertions read as URLs + /// rather than percent-encoded soup. + /// + private static string ServiceParameter(string redirectUrl) + { + const string marker = "service="; + int start = redirectUrl.IndexOf(marker, StringComparison.Ordinal); + Assert.True(start >= 0, $"No service parameter in '{redirectUrl}'."); + + return WebUtility.UrlDecode(redirectUrl[(start + marker.Length)..]); + } + + private static HomeController CreateController(string host, string pathBase) + { + var publicUrl = new PublicUrlService( + Options.Create(new PublicUrlOptions { PublicBaseUrl = PublicBaseUrl }), + Substitute.For()); + + var controller = new HomeController( + Substitute.For(), + Options.Create(new CasSettings { CasBaseUrl = CasBaseUrl }), + publicUrl, + Substitute.For(), + Substitute.For(), + Substitute.For()); + + var httpContext = new DefaultHttpContext + { + RequestServices = AuthenticationServices() + }; + httpContext.Request.Scheme = "https"; + httpContext.Request.Host = new HostString(host); + httpContext.Request.PathBase = new PathString(pathBase); + httpContext.Request.Path = new PathString("/Login"); + + controller.ControllerContext = new ControllerContext { HttpContext = httpContext }; + return controller; + } + + // Logout signs the cookie out, which resolves IAuthenticationService from the request. + private static IServiceProvider AuthenticationServices() + { + var authentication = Substitute.For(); + var services = Substitute.For(); + services.GetService(typeof(IAuthenticationService)).Returns(authentication); + return services; + } +} diff --git a/test/Classes/PublicUrlServiceTests.cs b/test/Classes/PublicUrlServiceTests.cs new file mode 100644 index 000000000..4f5b2c5e2 --- /dev/null +++ b/test/Classes/PublicUrlServiceTests.cs @@ -0,0 +1,185 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Options; +using NSubstitute; +using NSubstitute.ReturnsExtensions; +using Viper.Classes; + +namespace Viper.test.Classes; + +/// +/// The canonical public origin must come from configuration in deployed environments so a +/// forged Host header cannot influence a CAS callback. Development keeps the request-derived +/// fallback because the local port is dynamic. +/// +public class PublicUrlServiceTests +{ + private const string TestBaseUrl = "https://secure-test.vetmed.ucdavis.edu/2"; + private const string ProductionBaseUrl = "https://viper.vetmed.ucdavis.edu/2"; + + [Fact] + public void BaseUrl_ConfiguredOriginWins_OverForgedHostHeader() + { + var service = CreateService(TestBaseUrl, host: "attacker.example", pathBase: "/2"); + + Assert.Equal(TestBaseUrl, service.BaseUrl); + } + + [Fact] + public void BuildUrl_ConfiguredOriginWins_OverForgedHostHeader() + { + var service = CreateService(ProductionBaseUrl, host: "attacker.example", pathBase: "/2"); + + Assert.Equal($"{ProductionBaseUrl}/CasLogin", service.BuildUrl("/CasLogin")); + Assert.DoesNotContain("attacker.example", service.BuildUrl("/CasLogin"), StringComparison.OrdinalIgnoreCase); + } + + [Theory] + [InlineData("https://viper.vetmed.ucdavis.edu/2/", "https://viper.vetmed.ucdavis.edu/2")] + [InlineData(" https://viper.vetmed.ucdavis.edu/2 ", "https://viper.vetmed.ucdavis.edu/2")] + [InlineData("https://viper.vetmed.ucdavis.edu/", "https://viper.vetmed.ucdavis.edu")] + public void NormalizeBaseUrl_TrimsWhitespaceAndTrailingSlash(string configured, string expected) + { + Assert.Equal(expected, PublicUrlService.NormalizeBaseUrl(configured)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void NormalizeBaseUrl_BlankIsNull(string? configured) + { + Assert.Null(PublicUrlService.NormalizeBaseUrl(configured)); + } + + [Fact] + public void BuildUrl_AddsSeparator_WhenPathHasNoLeadingSlash() + { + var service = CreateService(TestBaseUrl, host: "secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + Assert.Equal($"{TestBaseUrl}/CasLogin", service.BuildUrl("CasLogin")); + } + + [Fact] + public void BuildUrl_EmptyPath_ReturnsBaseUrl() + { + var service = CreateService(TestBaseUrl, host: "secure-test.vetmed.ucdavis.edu", pathBase: "/2"); + + Assert.Equal(TestBaseUrl, service.BuildUrl(string.Empty)); + } + + [Fact] + public void BaseUrl_Unconfigured_FallsBackToRequestIncludingPathBase() + { + // Development only: no PublicBaseUrl set, so the origin comes from the request. + var service = CreateService(configured: null, host: "localhost:7157", pathBase: "/2"); + + Assert.Equal("https://localhost:7157/2", service.BaseUrl); + } + + [Fact] + public void BaseUrl_Unconfigured_NoPathBase_ReturnsOriginOnly() + { + var service = CreateService(configured: null, host: "localhost:7157", pathBase: string.Empty); + + Assert.Equal("https://localhost:7157", service.BaseUrl); + } + + [Fact] + public void BaseUrl_Unconfigured_NoRequest_FallsBackToLocalDevelopmentOrigin() + { + // Development background work (Hangfire email) has no request to derive from. Deployed + // environments never reach this because startup validation requires the configured value. + var accessor = Substitute.For(); + accessor.HttpContext.ReturnsNull(); + var service = new PublicUrlService(Options.Create(new PublicUrlOptions()), accessor); + + // The origin is resolved once into a static readonly field, so setting the variable here + // would be too late to affect it. Mirror the production rule instead, which keeps the + // expectation right for a missing, non-numeric or out-of-range value alike. + string? httpsPort = Environment.GetEnvironmentVariable("ASPNETCORE_HTTPS_PORT"); + int expectedPort = int.TryParse(httpsPort, out int parsed) && parsed > 0 && parsed < 65536 ? parsed : 7157; + + Assert.Equal($"https://localhost:{expectedPort}", service.BaseUrl); + } + + [Fact] + public void BaseUrl_Configured_NoRequest_StillUsesTheCanonicalOrigin() + { + // The email path must not pick up the local development origin in a deployed environment. + var accessor = Substitute.For(); + accessor.HttpContext.ReturnsNull(); + var service = new PublicUrlService(Options.Create(new PublicUrlOptions { PublicBaseUrl = ProductionBaseUrl }), accessor); + + Assert.Equal(ProductionBaseUrl, service.BaseUrl); + } + + #region Startup validation + + [Theory] + [InlineData(TestBaseUrl)] + [InlineData(ProductionBaseUrl)] + [InlineData("https://viper.vetmed.ucdavis.edu")] + public void Validate_AcceptsCanonicalDeployedUrls(string configured) + { + Assert.True(PublicUrlOptionsValidator.ValidateBaseUrl(configured, isDevelopment: false).Succeeded); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void Validate_MissingOutsideDevelopment_FailsStartup(string? configured) + { + var result = PublicUrlOptionsValidator.ValidateBaseUrl(configured, isDevelopment: false); + + Assert.True(result.Failed); + Assert.Contains("Application:PublicBaseUrl", result.FailureMessage, StringComparison.Ordinal); + } + + [Fact] + public void Validate_MissingInDevelopment_Succeeds() + { + // Development derives the origin from the request so dynamic local ports keep working. + Assert.True(PublicUrlOptionsValidator.ValidateBaseUrl(null, isDevelopment: true).Succeeded); + } + + [Fact] + public void Validate_HttpOutsideDevelopment_Fails() + { + Assert.True(PublicUrlOptionsValidator.ValidateBaseUrl("http://viper.vetmed.ucdavis.edu/2", isDevelopment: false).Failed); + } + + [Fact] + public void Validate_HttpInDevelopment_Succeeds() + { + Assert.True(PublicUrlOptionsValidator.ValidateBaseUrl("http://localhost:5000", isDevelopment: true).Succeeded); + } + + [Theory] + [InlineData("/2")] + [InlineData("viper.vetmed.ucdavis.edu/2")] + [InlineData("https://user:pass@viper.vetmed.ucdavis.edu/2")] + [InlineData("https://viper.vetmed.ucdavis.edu/2?next=x")] + [InlineData("https://viper.vetmed.ucdavis.edu/2#frag")] + [InlineData("https://viper.vetmed.ucdavis.edu/2?")] + [InlineData("https://viper.vetmed.ucdavis.edu/2#")] + public void Validate_RejectsMalformedOrUnsafeValues(string configured) + { + Assert.True(PublicUrlOptionsValidator.ValidateBaseUrl(configured, isDevelopment: false).Failed); + } + + #endregion + + private static PublicUrlService CreateService(string? configured, string host, string pathBase) + { + var context = new DefaultHttpContext(); + context.Request.Scheme = "https"; + context.Request.Host = new HostString(host); + context.Request.PathBase = new PathString(pathBase); + context.Request.Path = new PathString("/CasLogin"); + + var accessor = Substitute.For(); + accessor.HttpContext.Returns(context); + + return new PublicUrlService(Options.Create(new PublicUrlOptions { PublicBaseUrl = configured }), accessor); + } +} diff --git a/test/ClinicalScheduler/EmailNotificationTest.cs b/test/ClinicalScheduler/EmailNotificationTest.cs index 7dd97c420..1727ef320 100644 --- a/test/ClinicalScheduler/EmailNotificationTest.cs +++ b/test/ClinicalScheduler/EmailNotificationTest.cs @@ -6,6 +6,7 @@ using NSubstitute.ExceptionExtensions; using Viper.Areas.ClinicalScheduler.EmailTemplates.Models; using Viper.Areas.ClinicalScheduler.Services; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.EmailTemplates.Services; using Viper.Models.ClinicalScheduler; @@ -79,8 +80,9 @@ public EmailNotificationTest() .Returns(currentYear); // Setup email settings - var mockEmailSettingsOptions = Substitute.For>(); - mockEmailSettingsOptions.Value.Returns(new EmailSettings { BaseUrl = "https://test.example.com" }); + var mockPublicUrl = Substitute.For(); + mockPublicUrl.BaseUrl.Returns("https://test.example.com"); + mockPublicUrl.BuildUrl(Arg.Any()).Returns(ci => "https://test.example.com" + ci.Arg()); // Setup audit service _mockAuditService.LogInstructorRemovedAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) @@ -92,7 +94,7 @@ public EmailNotificationTest() _mockLogger, _mockEmailService, _mockEmailNotificationOptions, - mockEmailSettingsOptions, + mockPublicUrl, _mockGradYearService, _mockPermissionValidator, _mockEmailTemplateRenderer); @@ -560,8 +562,9 @@ public async Task RemoveInstructorScheduleAsync_MultipleEmailRecipients_SendsToA } }; _mockEmailNotificationOptions.Value.Returns(emailNotificationSettings); - var mockEmailSettingsOptions = Substitute.For>(); - mockEmailSettingsOptions.Value.Returns(new EmailSettings { BaseUrl = "https://test.example.com" }); + var mockPublicUrl = Substitute.For(); + mockPublicUrl.BaseUrl.Returns("https://test.example.com"); + mockPublicUrl.BuildUrl(Arg.Any()).Returns(ci => "https://test.example.com" + ci.Arg()); // Create a new service instance with the updated configuration var serviceWithMultipleRecipients = new TestableScheduleEditService( @@ -570,7 +573,7 @@ public async Task RemoveInstructorScheduleAsync_MultipleEmailRecipients_SendsToA _mockLogger, _mockEmailService, _mockEmailNotificationOptions, - mockEmailSettingsOptions, + mockPublicUrl, _mockGradYearService, _mockPermissionValidator, _mockEmailTemplateRenderer); diff --git a/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs b/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs index 9bb83e89c..d7bf6692d 100644 --- a/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs +++ b/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs @@ -14,6 +14,8 @@ using Viper.Services; using CS = Viper.Models.ClinicalScheduler; +using Viper.Classes; + namespace Viper.test.ClinicalScheduler.Integration { /// @@ -55,8 +57,8 @@ public ControllerServiceIntegrationTest() var mockEmailService = Substitute.For(); var mockEmailNotificationSettings = Substitute.For>(); mockEmailNotificationSettings.Value.Returns(new EmailNotificationSettings()); - var mockEmailSettings = Substitute.For>(); - mockEmailSettings.Value.Returns(new EmailSettings()); + var mockPublicUrl = Substitute.For(); + mockPublicUrl.BaseUrl.Returns("https://test.example.com"); var mockGradYearService = Substitute.For(); var mockPermissionValidator = Substitute.For(); var mockEmailTemplateRenderer = Substitute.For(); @@ -67,7 +69,7 @@ public ControllerServiceIntegrationTest() scheduleEditLogger, mockEmailService, mockEmailNotificationSettings, - mockEmailSettings, + mockPublicUrl, mockGradYearService, mockPermissionValidator, mockEmailTemplateRenderer); diff --git a/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs b/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs index 7d08884f6..55ad8daec 100644 --- a/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs +++ b/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs @@ -5,6 +5,7 @@ using NSubstitute; using NSubstitute.ExceptionExtensions; using Viper.Areas.ClinicalScheduler.Services; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.EmailTemplates.Services; using Viper.Services; @@ -56,8 +57,9 @@ public ScheduleEditServiceRollbackTest() var emailNotificationOptions = Substitute.For>(); emailNotificationOptions.Value.Returns(new EmailNotificationSettings()); - var emailSettingsOptions = Substitute.For>(); - emailSettingsOptions.Value.Returns(new EmailSettings()); + var publicUrl = Substitute.For(); + publicUrl.BaseUrl.Returns("https://test.example.com"); + publicUrl.BuildUrl(Arg.Any()).Returns(ci => "https://test.example.com" + ci.Arg()); _service = new ScheduleEditService( _context, @@ -65,7 +67,7 @@ public ScheduleEditServiceRollbackTest() Substitute.For>(), Substitute.For(), emailNotificationOptions, - emailSettingsOptions, + publicUrl, gradYearService, permissionValidator, Substitute.For()); diff --git a/test/ClinicalScheduler/ScheduleEditServiceTest.cs b/test/ClinicalScheduler/ScheduleEditServiceTest.cs index a05887886..add6303ee 100644 --- a/test/ClinicalScheduler/ScheduleEditServiceTest.cs +++ b/test/ClinicalScheduler/ScheduleEditServiceTest.cs @@ -6,6 +6,7 @@ using NSubstitute.ExceptionExtensions; using Viper.Areas.ClinicalScheduler.EmailTemplates.Models; using Viper.Areas.ClinicalScheduler.Services; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.EmailTemplates.Services; using Viper.Models.ClinicalScheduler; @@ -96,8 +97,9 @@ public ScheduleEditServiceTest() SeedTestData(); // Setup email settings - var mockEmailSettingsOptions = Substitute.For>(); - mockEmailSettingsOptions.Value.Returns(new EmailSettings { BaseUrl = "https://test.example.com" }); + var mockPublicUrl = Substitute.For(); + mockPublicUrl.BaseUrl.Returns("https://test.example.com"); + mockPublicUrl.BuildUrl(Arg.Any()).Returns(ci => "https://test.example.com" + ci.Arg()); _service = new TestableScheduleEditService( _context, @@ -105,7 +107,7 @@ public ScheduleEditServiceTest() _mockLogger, _mockEmailService, _mockEmailNotificationOptions, - mockEmailSettingsOptions, + mockPublicUrl, _mockGradYearService, _mockPermissionValidator, _mockEmailTemplateRenderer); diff --git a/test/ClinicalScheduler/TestDataBuilder.cs b/test/ClinicalScheduler/TestDataBuilder.cs index 9168671b4..0293df926 100644 --- a/test/ClinicalScheduler/TestDataBuilder.cs +++ b/test/ClinicalScheduler/TestDataBuilder.cs @@ -266,7 +266,7 @@ public static RAPSContext CreateRAPSContext() // Setup HttpHelper.Cache for UserHelper permission caching var memoryCache = new MemoryCache( new MemoryCacheOptions()); - HttpHelper.Configure(memoryCache, null!, null!, null!, null!, null!); + HttpHelper.Configure(memoryCache, null!, null!, null!, null!, null!, null!); // Create standard test permissions var permissions = new List diff --git a/test/ClinicalScheduler/TestableScheduleEditService.cs b/test/ClinicalScheduler/TestableScheduleEditService.cs index 51a2443fc..b79b65152 100644 --- a/test/ClinicalScheduler/TestableScheduleEditService.cs +++ b/test/ClinicalScheduler/TestableScheduleEditService.cs @@ -2,6 +2,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Viper.Areas.ClinicalScheduler.Services; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.EmailTemplates.Services; using Viper.Services; @@ -19,11 +20,11 @@ public TestableScheduleEditService( ILogger logger, IEmailService emailService, IOptions emailNotificationOptions, - IOptions emailSettingsOptions, + IPublicUrlService publicUrl, IGradYearService gradYearService, IPermissionValidator permissionValidator, IEmailTemplateRenderer emailTemplateRenderer) - : base(context, auditService, logger, emailService, emailNotificationOptions, emailSettingsOptions, gradYearService, permissionValidator, emailTemplateRenderer) + : base(context, auditService, logger, emailService, emailNotificationOptions, publicUrl, gradYearService, permissionValidator, emailTemplateRenderer) { } diff --git a/test/Effort/EffortIntegrationTestBase.cs b/test/Effort/EffortIntegrationTestBase.cs index 6cacfbc80..fad8e8dbf 100644 --- a/test/Effort/EffortIntegrationTestBase.cs +++ b/test/Effort/EffortIntegrationTestBase.cs @@ -72,7 +72,7 @@ private static RAPSContext CreateRAPSContext() // Setup HttpHelper.Cache for UserHelper permission caching var memoryCache = new MemoryCache(new MemoryCacheOptions()); - HttpHelper.Configure(memoryCache, null!, null!, null!, null!, null!); + HttpHelper.Configure(memoryCache, null!, null!, null!, null!, null!, null!); // Create standard Effort permissions var permissions = new List diff --git a/test/Effort/VerificationServiceTests.cs b/test/Effort/VerificationServiceTests.cs index 3d6273af7..3aa711af5 100644 --- a/test/Effort/VerificationServiceTests.cs +++ b/test/Effort/VerificationServiceTests.cs @@ -10,6 +10,7 @@ using Viper.Areas.Effort.Models.DTOs.Responses; using Viper.Areas.Effort.Models.Entities; using Viper.Areas.Effort.Services; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.EmailTemplates.Services; using Viper.Models.VIPER; @@ -66,11 +67,9 @@ public VerificationServiceTests() }; var settingsOptions = Options.Create(_settings); - var emailSettings = new EmailSettings - { - BaseUrl = "https://test.example.com" - }; - var emailSettingsOptions = Options.Create(emailSettings); + var publicUrl = Substitute.For(); + publicUrl.BaseUrl.Returns("https://test.example.com"); + publicUrl.BuildUrl(Arg.Any()).Returns(ci => "https://test.example.com" + ci.Arg()); _emailTemplateRendererMock = Substitute.For(); _emailTemplateRendererMock @@ -102,7 +101,7 @@ public VerificationServiceTests() _classificationServiceMock, _loggerMock, settingsOptions, - emailSettingsOptions, + publicUrl, _emailTemplateRendererMock); SeedTestData(); @@ -661,50 +660,6 @@ await _auditServiceMock.Received(1).LogPersonChangeAsync( Arg.Is(x => x == null), Arg.Is(o => o.ToString()!.Contains("Failed")), Arg.Any()); } - [Fact] - public async Task SendVerificationEmailAsync_ReturnsError_WhenBaseUrlNotConfigured() - { - // Arrange: Create service with missing BaseUrl configuration - var badSettings = new EffortSettings - { - VerificationEmailSubject = "Please Verify Your Effort", - VerificationReplyDays = 7 - }; - var badEmailSettings = new EmailSettings - { - BaseUrl = "" // Missing/empty BaseUrl - }; - - var serviceWithBadConfig = new VerificationService( - _context, - _viperContext, - _auditServiceMock, - _permissionServiceMock, - _termServiceMock, - _emailServiceMock, - _classificationServiceMock, - _loggerMock, - Options.Create(badSettings), - Options.Create(badEmailSettings), - _emailTemplateRendererMock); - - _permissionServiceMock.GetCurrentUserEmail().Returns("sender@ucdavis.edu"); - - // Act - var result = await serviceWithBadConfig.SendVerificationEmailAsync(TestPersonId, TestTermCode, TestContext.Current.CancellationToken); - - // Assert - Assert.False(result.Success); - Assert.Equal("Email system configuration error. Please contact support.", result.Error); - - // Verify audit was logged for the configuration failure - await _auditServiceMock.Received(1).LogPersonChangeAsync( - TestPersonId, TestTermCode, EffortAuditActions.VerifyEmail, - Arg.Is(x => x == null), Arg.Is(o => o.ToString()!.Contains("Configuration error")), Arg.Any()); - // Verify no email was attempted - await _emailServiceMock.DidNotReceive().SendEmailAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); - } - [Fact] public async Task SendVerificationEmailAsync_Succeeds_WhenValidEmail() { diff --git a/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs b/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs index 9f9643b3c..cf8a70e53 100644 --- a/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs +++ b/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs @@ -3,6 +3,7 @@ using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Options; using Viper.Areas.ClinicalScheduler.EmailTemplates.Models; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.Classes.Utilities; using Viper.EmailTemplates.Services; @@ -21,7 +22,7 @@ public class ScheduleEditService : IScheduleEditService private readonly ILogger _logger; private readonly IEmailService _emailService; private readonly EmailNotificationSettings _emailNotificationSettings; - private readonly EmailSettings _emailSettings; + private readonly IPublicUrlService _publicUrl; private readonly IGradYearService _gradYearService; private readonly IPermissionValidator _permissionValidator; private readonly IEmailTemplateRenderer _emailTemplateRenderer; @@ -32,7 +33,7 @@ public ScheduleEditService( ILogger logger, IEmailService emailService, IOptions emailNotificationOptions, - IOptions emailSettingsOptions, + IPublicUrlService publicUrl, IGradYearService gradYearService, IPermissionValidator permissionValidator, IEmailTemplateRenderer emailTemplateRenderer) @@ -42,7 +43,7 @@ public ScheduleEditService( _logger = logger; _emailService = emailService; _emailNotificationSettings = emailNotificationOptions.Value; - _emailSettings = emailSettingsOptions.Value; + _publicUrl = publicUrl; _gradYearService = gradYearService; _permissionValidator = permissionValidator; _emailTemplateRenderer = emailTemplateRenderer; @@ -655,8 +656,6 @@ private async Task SendPrimaryEvaluatorRemovedNotificationAsync(InstructorSchedu LogSanitizer.SanitizeId(schedule.MothraId), LogSanitizer.SanitizeId(newPrimaryMothraId), schedule.RotationId, schedule.WeekId); return; } - // Get base URL for links - var baseUrl = string.IsNullOrWhiteSpace(_emailSettings.BaseUrl) ? null : _emailSettings.BaseUrl; // Get instructor information var instructorName = "Unknown Instructor"; @@ -744,15 +743,13 @@ await _context.Entry(schedule) // Use the passed requiresPrimaryEvaluator parameter (determined by frontend) // Build rotation link - var rotationLink = baseUrl is null - ? $"/ClinicalScheduler/rotation/{schedule.RotationId}" - : $"{baseUrl}/ClinicalScheduler/rotation/{schedule.RotationId}"; + var rotationLink = _publicUrl.BuildUrl($"/ClinicalScheduler/rotation/{schedule.RotationId}"); // Build email subject and body using Razor template var emailSubject = $"Primary Evaluator Removed - {rotationName} - Week {weekNumber}"; var viewModel = new PrimaryEvaluatorRemovedViewModel { - BaseUrl = baseUrl ?? "", + BaseUrl = _publicUrl.BaseUrl, InstructorName = instructorName, RotationName = rotationName, RotationLink = rotationLink, diff --git a/web/Areas/Effort/Services/VerificationService.cs b/web/Areas/Effort/Services/VerificationService.cs index 5ab438658..e9607e742 100644 --- a/web/Areas/Effort/Services/VerificationService.cs +++ b/web/Areas/Effort/Services/VerificationService.cs @@ -7,6 +7,7 @@ using Viper.Areas.Effort.Models; using Viper.Areas.Effort.Models.DTOs.Responses; using Viper.Areas.Effort.Models.Entities; +using Viper.Classes; using Viper.Classes.SQLContext; using Viper.Classes.Utilities; using Viper.EmailTemplates.Services; @@ -29,7 +30,7 @@ public class VerificationService : IVerificationService private readonly ICourseClassificationService _classificationService; private readonly ILogger _logger; private readonly EffortSettings _settings; - private readonly EmailSettings _emailSettings; + private readonly IPublicUrlService _publicUrl; private readonly IEmailTemplateRenderer _emailTemplateRenderer; public VerificationService( @@ -42,7 +43,7 @@ public VerificationService( ICourseClassificationService classificationService, ILogger logger, IOptions settings, - IOptions emailSettings, + IPublicUrlService publicUrl, IEmailTemplateRenderer emailTemplateRenderer) { _context = context; @@ -54,7 +55,7 @@ public VerificationService( _classificationService = classificationService; _logger = logger; _settings = settings.Value; - _emailSettings = emailSettings.Value; + _publicUrl = publicUrl; _emailTemplateRenderer = emailTemplateRenderer; } @@ -374,27 +375,7 @@ await _auditService.LogPersonChangeAsync( return new EmailSendResult { Success = false, Error = "Invalid email address" }; } - string verificationUrl; - try - { - verificationUrl = BuildVerificationUrl(termCode); - } - catch (InvalidOperationException ex) - { - _logger.LogError(ex, "Configuration error building verification URL for term {TermCode}", termCode); - - var configErrorAuditData = new - { - RecipientPersonId = personId, - RecipientName = $"{instructor.LastName}, {instructor.FirstName}", - SendResult = "Failed: Configuration error" - }; - - await _auditService.LogPersonChangeAsync( - personId, termCode, EffortAuditActions.VerifyEmail, null, configErrorAuditData, ct); - - return new EmailSendResult { Success = false, Error = "Email system configuration error. Please contact support." }; - } + string verificationUrl = BuildVerificationUrl(termCode); try { @@ -716,22 +697,10 @@ private async Task> GetCourseRelationshipsAsync( .ToListAsync(ct); } + // Built from the canonical origin, never the request Host. PublicUrlOptionsValidator + // already proved that origin is an absolute https URL at startup. private string BuildVerificationUrl(int termCode) - { - // Require configured base URL to avoid Host header injection - if (string.IsNullOrWhiteSpace(_emailSettings.BaseUrl)) - { - throw new InvalidOperationException("EmailSettings:BaseUrl must be configured for verification emails."); - } - - var baseUrlNormalized = _emailSettings.BaseUrl.TrimEnd('/') + "/"; - if (!Uri.TryCreate(baseUrlNormalized, UriKind.Absolute, out var baseUri)) - { - throw new InvalidOperationException($"EmailSettings:BaseUrl value '{_emailSettings.BaseUrl}' is not a valid absolute URL."); - } - - return new Uri(baseUri, $"Effort/{termCode}/my-effort").ToString(); - } + => _publicUrl.BuildUrl($"/Effort/{termCode}/my-effort"); /// /// Determines if an effort record has zero effort value. @@ -846,7 +815,7 @@ private VerificationReminderViewModel BuildVerificationEmailViewModel( return new VerificationReminderViewModel { - BaseUrl = _emailSettings.BaseUrl ?? "", + BaseUrl = _publicUrl.BaseUrl, TermDescription = termDescription, TermStartDate = termStartDate, TermEndDate = termEndDate, diff --git a/web/Classes/HealthChecks/HealthCheckExtensions.cs b/web/Classes/HealthChecks/HealthCheckExtensions.cs index d11df6c7d..bd34f2e87 100644 --- a/web/Classes/HealthChecks/HealthCheckExtensions.cs +++ b/web/Classes/HealthChecks/HealthCheckExtensions.cs @@ -232,9 +232,10 @@ public static IServiceCollection AddViperHealthChecks( // UseApiEndpointDelegatingHandler below) so the endpoint filter // can recognize the self-call without widening the IP allowlist // to cover whatever NAT'd source IP the loop-out produces. - // Dev has no BaseUrl configured, so fall back to a relative URL. - var baseUrl = configuration["EmailSettings:BaseUrl"]?.TrimEnd('/'); - var healthEndpointUrl = string.IsNullOrWhiteSpace(baseUrl) + // Dev leaves the canonical origin unset, so fall back to a relative URL. + var baseUrl = PublicUrlService.NormalizeBaseUrl( + configuration[$"{PublicUrlOptions.SectionName}:{nameof(PublicUrlOptions.PublicBaseUrl)}"]); + var healthEndpointUrl = baseUrl is null ? "/health/detail" : $"{baseUrl}/health/detail"; services.AddTransient(); diff --git a/web/Classes/HttpHelper.cs b/web/Classes/HttpHelper.cs index ffde52032..ebaf57497 100644 --- a/web/Classes/HttpHelper.cs +++ b/web/Classes/HttpHelper.cs @@ -1,9 +1,9 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.DataProtection; -using Microsoft.AspNetCore.Http.Extensions; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Caching.Memory; using NLog; +using Viper.Classes; namespace Viper { @@ -16,18 +16,20 @@ public static class HttpHelper private static IHttpContextAccessor? httpContextAccessor; private static IAuthorizationService? authorizationService; private static IDataProtectionProvider? dataProtectionProvider; + private static IPublicUrlService? publicUrlService; /// - /// Configures the helper with system-wide services (memory cache, configuration, environment, context accessor, authorization, data protection) + /// Configures the helper with system-wide services (memory cache, configuration, environment, context accessor, authorization, data protection, public URL) /// - public static void Configure(IMemoryCache? memoryCache, IConfiguration? configurationSettings, IWebHostEnvironment env, IHttpContextAccessor? httpContextAccessor, IAuthorizationService? authorizationService, IDataProtectionProvider? dataProtectionProvider) + public static void Configure(IMemoryCache? memoryCache, IConfiguration? configurationSettings, IWebHostEnvironment env, IHttpContextAccessor? contextAccessor, IAuthorizationService? authService, IDataProtectionProvider? dataProtection, IPublicUrlService? publicUrl) { Cache = memoryCache; Settings = configurationSettings; Environment = env; - HttpHelper.httpContextAccessor = httpContextAccessor; - HttpHelper.authorizationService = authorizationService; - HttpHelper.dataProtectionProvider = dataProtectionProvider; + httpContextAccessor = contextAccessor; + authorizationService = authService; + dataProtectionProvider = dataProtection; + publicUrlService = publicUrl; } /// @@ -77,27 +79,13 @@ public static HttpContext? HttpContext public static IDataProtectionProvider? DataProtectionProvider { get { return dataProtectionProvider; } } /// - /// Gets the root URL including protocol and port for Viper.Net + /// Gets the root URL including protocol and port for Viper.Net. Deployed environments + /// return the configured canonical origin (Application:PublicBaseUrl); Development + /// derives it from the request. See . /// public static string GetRootURL() { - string rootURL = String.Empty; - - HttpRequest? thisRequest = httpContextAccessor?.HttpContext?.Request; - - if (thisRequest != null) - { - Uri url = new(thisRequest.GetDisplayUrl()); - rootURL = url.GetLeftPart(UriPartial.Authority); - - if (url.AbsolutePath.StartsWith("/2/")) - { - rootURL += "/2"; - } - - } - - return rootURL ?? String.Empty; + return publicUrlService?.BaseUrl ?? string.Empty; } /// /// Gets the root URL for ColdFusion Viper based off the enviroment diff --git a/web/Classes/PublicUrlService.cs b/web/Classes/PublicUrlService.cs new file mode 100644 index 000000000..bfa073837 --- /dev/null +++ b/web/Classes/PublicUrlService.cs @@ -0,0 +1,174 @@ +using Microsoft.AspNetCore.Http.Extensions; +using Microsoft.Extensions.Options; + +namespace Viper.Classes +{ + /// + /// Canonical public origin for this deployment, bound from the "Application" configuration + /// section. Deployed environments must set it; Development derives the origin from the + /// request so the dynamic local port keeps working. + /// + public class PublicUrlOptions + { + public const string SectionName = "Application"; + + /// + /// Absolute base URL including scheme, host, optional port and PathBase, e.g. + /// "https://viper.vetmed.ucdavis.edu/2". + /// + public string? PublicBaseUrl { get; set; } + } + + /// + /// Supplies the origin for URLs that leave the application (CAS service callbacks, sitemap + /// entries, emulation links). Deployed environments read it from configuration so a forged + /// Host header cannot influence a security callback. + /// + public interface IPublicUrlService + { + /// + /// Canonical base URL with no trailing slash, e.g. "https://viper.vetmed.ucdavis.edu/2". + /// + string BaseUrl { get; } + + /// + /// Canonical base URL plus an application-relative path, e.g. BuildUrl("/CasLogin"). + /// + string BuildUrl(string relativePath); + } + + /// + public class PublicUrlService : IPublicUrlService + { + private readonly string? _configuredBaseUrl; + private readonly IHttpContextAccessor _httpContextAccessor; + + public PublicUrlService(IOptions options, IHttpContextAccessor httpContextAccessor) + { + _configuredBaseUrl = NormalizeBaseUrl(options.Value.PublicBaseUrl); + _httpContextAccessor = httpContextAccessor; + } + + public string BaseUrl + { + get + { + if (_configuredBaseUrl != null) + { + return _configuredBaseUrl; + } + + HttpRequest? request = _httpContextAccessor.HttpContext?.Request; + return request != null ? FromRequest(request) : LocalDevelopmentOrigin; + } + } + + public string BuildUrl(string relativePath) + { + if (string.IsNullOrEmpty(relativePath)) + { + return BaseUrl; + } + + return BaseUrl + (relativePath.StartsWith('/') ? relativePath : "/" + relativePath); + } + + /// + /// Trims whitespace and any trailing slash so callers can append "/Path" unconditionally. + /// Returns null when nothing is configured. + /// + public static string? NormalizeBaseUrl(string? configured) + { + return string.IsNullOrWhiteSpace(configured) ? null : configured.Trim().TrimEnd('/'); + } + + /// + /// Last resort for Development work that has no request to derive from, such as email + /// sent from a background job. Resolved once because process environment variables do + /// not change after start. Deployed environments never reach it because + /// PublicUrlOptionsValidator fails startup when the canonical origin is missing. + /// + private static readonly string LocalDevelopmentOrigin = BuildLocalDevelopmentOrigin(); + + private static string BuildLocalDevelopmentOrigin() + { + const int defaultPort = 7157; + string? httpsPort = Environment.GetEnvironmentVariable("ASPNETCORE_HTTPS_PORT"); + int port = int.TryParse(httpsPort, out int parsed) && parsed > 0 && parsed < 65536 ? parsed : defaultPort; + return $"https://localhost:{port}"; + } + + /// + /// Development fallback: derive the origin from the current request, preserving the + /// PathBase. Deployed environments never reach this because PublicUrlOptionsValidator + /// fails startup when the setting is missing. + /// + private static string FromRequest(HttpRequest request) + { + string origin = new Uri(request.GetDisplayUrl()).GetLeftPart(UriPartial.Authority); + return origin + request.PathBase.Value?.TrimEnd('/'); + } + } + + /// + /// Fails startup when a deployed environment has no usable canonical origin, so the app + /// cannot silently fall back to request-derived URLs for CAS callbacks. + /// + public class PublicUrlOptionsValidator : IValidateOptions + { + private readonly IWebHostEnvironment _environment; + + public PublicUrlOptionsValidator(IWebHostEnvironment environment) + { + _environment = environment; + } + + public ValidateOptionsResult Validate(string? name, PublicUrlOptions options) + { + return ValidateBaseUrl(options.PublicBaseUrl, _environment.IsDevelopment()); + } + + /// + /// Exposed for tests: applies the same rules the startup validator uses. + /// + public static ValidateOptionsResult ValidateBaseUrl(string? configured, bool isDevelopment) + { + const string setting = "Application:PublicBaseUrl"; + string? normalized = PublicUrlService.NormalizeBaseUrl(configured); + + if (normalized == null) + { + return isDevelopment + ? ValidateOptionsResult.Success + : ValidateOptionsResult.Fail($"{setting} is required outside Development. Set it to the canonical public URL, for example https://viper.vetmed.ucdavis.edu/2."); + } + + if (!Uri.TryCreate(normalized, UriKind.Absolute, out Uri? uri)) + { + return ValidateOptionsResult.Fail($"{setting} must be an absolute URL."); + } + + if (uri.Scheme != Uri.UriSchemeHttps && !(isDevelopment && uri.Scheme == Uri.UriSchemeHttp)) + { + return ValidateOptionsResult.Fail($"{setting} must use https outside Development."); + } + + if (!string.IsNullOrEmpty(uri.UserInfo)) + { + return ValidateOptionsResult.Fail($"{setting} must not contain user information."); + } + + if (!string.IsNullOrEmpty(uri.Query)) + { + return ValidateOptionsResult.Fail($"{setting} must not contain a query string."); + } + + if (!string.IsNullOrEmpty(uri.Fragment)) + { + return ValidateOptionsResult.Fail($"{setting} must not contain a fragment."); + } + + return ValidateOptionsResult.Success; + } + } +} diff --git a/web/Controllers/HomeController.cs b/web/Controllers/HomeController.cs index 1f7d613b7..2771dcc6d 100644 --- a/web/Controllers/HomeController.cs +++ b/web/Controllers/HomeController.cs @@ -8,7 +8,6 @@ using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.DataProtection; -using Microsoft.AspNetCore.Http.Extensions; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Extensions.Caching.Memory; @@ -36,13 +35,15 @@ public class HomeController : AreaController #pragma warning restore S5332 private readonly IHttpClientFactory _clientFactory; private readonly CasSettings _settings; + private readonly IPublicUrlService _publicUrl; private readonly List _casAttributesToCapture = new() { "authenticationDate", "credentialType" }; private readonly IUserHelper _userHelper; - public HomeController(IHttpClientFactory clientFactory, IOptions settingsOptions, AAUDContext aAUDContext, RAPSContext rapsContext, VIPERContext viperContext) + public HomeController(IHttpClientFactory clientFactory, IOptions settingsOptions, IPublicUrlService publicUrl, AAUDContext aAUDContext, RAPSContext rapsContext, VIPERContext viperContext) { this._clientFactory = clientFactory; this._settings = settingsOptions.Value; + this._publicUrl = publicUrl; this._aAUDContext = aAUDContext; this._rapsContext = rapsContext; this._viperContext = viperContext; @@ -94,16 +95,23 @@ private NavMenu Nav() [SearchExclude] public IActionResult Login([FromQuery] string? ReturnUrl = null) { - Uri url = new(Request.GetDisplayUrl()); - string baseURl = url.GetLeftPart(UriPartial.Authority); - string returnURL = HttpHelper.GetRootURL().Replace(baseURl, ""); + // Browsers and CAS don't understand "~", and leaving it on would also let a + // "~/api/..." ReturnUrl slip past the guard below. + ReturnUrl = NormalizeAppRelativeUrl(ReturnUrl); + + // Default to the application root under the deployed PathBase ("" locally, "/2" on TEST/PROD). + string returnURL = Request.PathBase.Value ?? string.Empty; if (!string.IsNullOrEmpty(ReturnUrl)) { returnURL = ReturnUrl; } - if (returnURL.StartsWith("/api")) + // Strip the PathBase (e.g. "/2") before the /api guard so a base-prefixed + // "/2/api/..." ReturnUrl can't slip past this root-relative check and get + // forwarded to CAS. + var apiCheckUrl = StripPathBase(returnURL, Request.PathBase.Value); + if (apiCheckUrl != null && IsApiPath(apiCheckUrl)) { return Unauthorized(); } @@ -113,6 +121,46 @@ public IActionResult Login([FromQuery] string? ReturnUrl = null) return new RedirectResult(authorizationEndpoint); } + // Url.IsLocalUrl accepts app-relative "~/..." URLs, but browsers and CAS don't + // understand the "~", so normalize "~/..." to "/..." before validating or + // redirecting. Leaves all other values (including null) unchanged. + private static string? NormalizeAppRelativeUrl(string? returnUrl) + => returnUrl != null && returnUrl.StartsWith("~/") ? returnUrl[1..] : returnUrl; + + // Routing is case-insensitive, so the /api guard must be too; matching on a segment + // boundary keeps non-API paths that merely start with "api" (e.g. "/apiary") out of + // the guard. internal (not private) so it is unit-testable via InternalsVisibleTo. + internal static bool IsApiPath(string url) + { + if (!url.StartsWith("/api", StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + return url.Length == 4 || url[4] is '/' or '?' or '#'; + } + + // Removes the application's PathBase prefix (e.g. "/2" in a subpath deployment) from a return + // URL so the splash classifier and label resolver can treat it as root-relative. Matches on a + // segment boundary so "/2" never strips from an unrelated "/22/...". Returns the URL unchanged + // when there is no base to strip (e.g. local dev, where PathBase is empty). + // internal (not private) so it is unit-testable via InternalsVisibleTo. + internal static string? StripPathBase(string? url, string? pathBase) + { + if (string.IsNullOrEmpty(url) || string.IsNullOrEmpty(pathBase)) + { + return url; + } + + if (url.StartsWith(pathBase, StringComparison.OrdinalIgnoreCase) + && (url.Length == pathBase.Length || url[pathBase.Length] is '/' or '?' or '#')) + { + return url[pathBase.Length..]; + } + + return url; + } + [Route("/[action]")] [SearchExclude] public IActionResult RefreshSession() @@ -260,7 +308,7 @@ public async Task Logout() await HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); // Send homepage link after CAS logout - var returnUrl = WebUtility.UrlEncode(HttpHelper.GetRootURL()); + var returnUrl = WebUtility.UrlEncode(_publicUrl.BaseUrl); return new RedirectResult(_settings.CasBaseUrl + "logout?service=" + returnUrl); } @@ -287,13 +335,14 @@ public IActionResult MyPermissions() /// - /// Utility function for creating redirect URLs + /// Utility function for creating redirect URLs. Built from the configured canonical + /// origin, never the request Host, so a forged Host cannot poison a CAS callback. /// /// /// Compiled URL - private static string BuildRedirectUri(string targetPath) + private string BuildRedirectUri(string targetPath) { - return HttpHelper.GetRootURL() + targetPath; + return _publicUrl.BuildUrl(targetPath); } /// diff --git a/web/Program.cs b/web/Program.cs index 126ab15c8..d1de32e47 100644 --- a/web/Program.cs +++ b/web/Program.cs @@ -149,6 +149,14 @@ // Add CAS settings from appSettings configuration builder.Services.Configure(builder.Configuration.GetSection("Cas")); + // Canonical public origin for CAS callbacks and other outward-facing links. Validated on + // start so a deployed environment fails fast instead of falling back to the request Host. + builder.Services.AddOptions() + .Bind(builder.Configuration.GetSection(PublicUrlOptions.SectionName)) + .ValidateOnStart(); + builder.Services.AddSingleton, PublicUrlOptionsValidator>(); + builder.Services.AddSingleton(); + // Define authorization policies builder.Services.AddAuthorization(options => { @@ -232,22 +240,6 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db builder.Services.Configure(builder.Configuration.GetSection("EffortSettings")); - // In development, derive BaseUrl from ASPNETCORE_HTTPS_PORT if not explicitly configured - if (builder.Environment.IsDevelopment()) - { - builder.Services.PostConfigure(settings => - { - if (string.IsNullOrWhiteSpace(settings.BaseUrl)) - { - var httpsPort = Environment.GetEnvironmentVariable("ASPNETCORE_HTTPS_PORT") ?? "7157"; - if (int.TryParse(httpsPort, out var port) && port > 0 && port < 65536) - { - settings.BaseUrl = $"https://localhost:{port}"; - } - } - }); - } - // Harvest phases (order matters for DI resolution, but phases self-order via Order property) builder.Services.AddScoped(); builder.Services.AddScoped(); @@ -526,7 +518,7 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db pattern: "{controller=Home}/{action=Index}").RequireAuthorization(); // Setup the memory cache so we can use it via a simple static method - HttpHelper.Configure(app.Services.GetService(), app.Services.GetService(), app.Environment, app.Services.GetService(), app.Services.GetService(), app.Services.GetService()); + HttpHelper.Configure(app.Services.GetService(), app.Services.GetService(), app.Environment, app.Services.GetService(), app.Services.GetService(), app.Services.GetService(), app.Services.GetRequiredService()); #pragma warning disable S6966 // app.Run() is appropriate for main entry point, not app.RunAsync() app.Run(); diff --git a/web/Services/EmailService.cs b/web/Services/EmailService.cs index 8c91b75d1..4b1e4e308 100644 --- a/web/Services/EmailService.cs +++ b/web/Services/EmailService.cs @@ -311,12 +311,6 @@ public class EmailSettings public string DefaultFromAddress { get; set; } = "noreply@example.com"; public bool UseMailpit { get; set; } = false; - /// - /// Base URL for links in emails (e.g., "https://viper.vetmed.ucdavis.edu/2"). - /// Used to construct absolute URLs for email content. - /// - public string? BaseUrl { get; set; } - /// /// When true, all emails are redirected to the logged-in user's email address. /// Use for non-production environments to allow testers to see emails their actions generate. diff --git a/web/appsettings.Production.json b/web/appsettings.Production.json index 8b96ae2a0..8277341d6 100644 --- a/web/appsettings.Production.json +++ b/web/appsettings.Production.json @@ -7,6 +7,14 @@ } }, "LoggingPath": "s:\\nlog", + // Only the hostnames this environment is actually reached by. Defence in depth behind + // Cloudflare/F5/IIS: CAS callbacks come from Application:PublicBaseUrl, not the Host header. + // localhost is kept so on-server probes and IIS itself are not rejected. + "AllowedHosts": "viper.vetmed.ucdavis.edu;localhost", + "Application": { + // Canonical public origin, including the /2 PathBase of the IIS sub-app. + "PublicBaseUrl": "https://viper.vetmed.ucdavis.edu/2" + }, "ConnectionStrings": { "AAUD": "", "Courses": "", @@ -29,8 +37,7 @@ "SmtpPort": 25, "EnableSsl": true, "DefaultFromAddress": "svmithelp@ucdavis.edu", - "UseMailpit": false, - "BaseUrl": "https://viper.vetmed.ucdavis.edu/2" + "UseMailpit": false }, "Hangfire": { "DashboardAppPath": "/2/Computing" diff --git a/web/appsettings.Test.json b/web/appsettings.Test.json index 9f0e2b4c5..c04521f48 100644 --- a/web/appsettings.Test.json +++ b/web/appsettings.Test.json @@ -7,6 +7,14 @@ } }, "LoggingPath": "s:\\nlog", + // Only the hostnames this environment is actually reached by. Defence in depth behind + // Cloudflare/F5/IIS: CAS callbacks come from Application:PublicBaseUrl, not the Host header. + // localhost is kept so on-server probes and IIS itself are not rejected. + "AllowedHosts": "secure-test.vetmed.ucdavis.edu;localhost", + "Application": { + // Canonical public origin, including the /2 PathBase of the IIS sub-app. + "PublicBaseUrl": "https://secure-test.vetmed.ucdavis.edu/2" + }, "ConnectionStrings": { "AAUD": "", "Courses": "", @@ -33,7 +41,6 @@ "EnableSsl": true, "DefaultFromAddress": "svmithelp@ucdavis.edu", "UseMailpit": false, - "BaseUrl": "https://secure-test.vetmed.ucdavis.edu/2", "RedirectToCurrentUser": true }, "EffortSettings": { From 6de4d1ada941abea5df973a58ea5ac53447d2bca Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 28 Aug 2026 01:06:12 -0700 Subject: [PATCH 2/2] fix(sitemap): stop AmbiguousMatchException turning sitemap.xml into a 404 PermissionAttribute derives from AuthorizeAttribute, so an action carrying both (HomeController.Policy, EmulateUser) matched AuthorizeAttribute twice and the singular GetCustomAttribute threw. The catch swallowed it and fell through, so /sitemap.xml answered 404 in every environment. - Ask for any matching attribute rather than exactly one; only presence was ever tested, so the filter behavior is unchanged - Drop the AuthorizeAttribute test, which that same inheritance made dead code: anything gated by [Permission] already fails the [Permission] test - Log the swallowed exception, which is why a total outage of the endpoint went unnoticed --- test/Classes/SitemapMiddlewareTests.cs | 147 +++++++++++++++++++++++++ web/Classes/SitemapMiddleware.cs | 68 +++++++----- 2 files changed, 190 insertions(+), 25 deletions(-) create mode 100644 test/Classes/SitemapMiddlewareTests.cs diff --git a/test/Classes/SitemapMiddlewareTests.cs b/test/Classes/SitemapMiddlewareTests.cs new file mode 100644 index 000000000..71691d7be --- /dev/null +++ b/test/Classes/SitemapMiddlewareTests.cs @@ -0,0 +1,147 @@ +using System.Reflection; +using System.Text; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging.Abstractions; +using Viper.Classes; +using Web.Authorization; + +namespace Viper.test.Classes; + +/// +/// The sitemap reflects over every controller action. PermissionAttribute derives from +/// AuthorizeAttribute, so an action carrying both matched AuthorizeAttribute twice and the +/// singular GetCustomAttribute threw AmbiguousMatchException. The catch swallowed it and the +/// endpoint fell through to a 404 in every environment. +/// +public class SitemapMiddlewareTests +{ + [Fact] + public async Task SitemapXml_Returns200Xml_NotAFallThrough() + { + bool nextCalled = false; + var middleware = new SitemapMiddleware(_ => { nextCalled = true; return Task.CompletedTask; }, NullLogger.Instance); + + var context = BuildContext("/sitemap.xml"); + using var body = new MemoryStream(); + context.Response.Body = body; + + await middleware.Invoke(context); + + Assert.False(nextCalled, "generation failed and fell through to the pipeline"); + Assert.Equal(200, context.Response.StatusCode); + Assert.Equal("application/xml", context.Response.ContentType); + + string xml = Encoding.UTF8.GetString(body.ToArray()); + Assert.StartsWith("", xml, StringComparison.Ordinal); + } + + [Fact] + public async Task SitemapXml_IncludesAnonymousActions() + { + var middleware = new SitemapMiddleware(_ => Task.CompletedTask, NullLogger.Instance); + + var context = BuildContext("/sitemap.xml"); + using var body = new MemoryStream(); + context.Response.Body = body; + + await middleware.Invoke(context); + + // An empty would mean the reflection walk bailed out without throwing. + string xml = Encoding.UTF8.GetString(body.ToArray()); + Assert.Contains("", xml, StringComparison.Ordinal); + + // HomeController.Index is [AllowAnonymous] with no gate, so it belongs in a public sitemap. + Assert.Contains("/home/index", xml, StringComparison.Ordinal); + } + + [Fact] + public async Task SitemapXml_ExcludesPermissionGatedActions() + { + var middleware = new SitemapMiddleware(_ => Task.CompletedTask, NullLogger.Instance); + + var context = BuildContext("/sitemap.xml"); + using var body = new MemoryStream(); + context.Response.Body = body; + + await middleware.Invoke(context); + + // EmulateUser carries [Authorize] and [Permission], the pairing that used to throw. + // It must stay out of a public sitemap. + string xml = Encoding.UTF8.GetString(body.ToArray()); + Assert.DoesNotContain("emulateuser", xml, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task OtherPaths_FallThroughUntouched() + { + bool nextCalled = false; + var middleware = new SitemapMiddleware(_ => { nextCalled = true; return Task.CompletedTask; }, NullLogger.Instance); + + await middleware.Invoke(BuildContext("/Directory")); + + Assert.True(nextCalled); + } + + [Fact] + public void IsPubliclyListable_ExcludesAnonymousActionOnPermissionGatedController() + { + // [Permission] is an IAuthorizationFilter, and only the built-in AuthorizeFilter honors + // [AllowAnonymous]. A class-gated action still forbids at runtime, so advertising it in + // the sitemap would point anonymous visitors at a 403. + Assert.False(SitemapMiddleware.IsPubliclyListable(MethodOf(nameof(GatedController.Anonymous)))); + } + + [Fact] + public void IsPubliclyListable_ExcludesAnonymousActionOnSearchExcludedController() + { + Assert.False(SitemapMiddleware.IsPubliclyListable(MethodOf(nameof(SearchExcludedController.Anonymous)))); + } + + [Fact] + public void IsPubliclyListable_IncludesUngatedAnonymousAction() + { + Assert.True(SitemapMiddleware.IsPubliclyListable(MethodOf(nameof(OpenController.Anonymous)))); + } + + [Fact] + public void IsPubliclyListable_ExcludesActionWithoutAllowAnonymous() + { + Assert.False(SitemapMiddleware.IsPubliclyListable(MethodOf(nameof(OpenController.RequiresLogin)))); + } + + private static MethodInfo MethodOf(string name) => typeof(T).GetMethod(name)!; + + [Permission(Allow = "SVMSecure.Test")] + private sealed class GatedController : Controller + { + [AllowAnonymous] + public IActionResult Anonymous() => Ok(); + } + + [SearchExclude] + private sealed class SearchExcludedController : Controller + { + [AllowAnonymous] + public IActionResult Anonymous() => Ok(); + } + + private sealed class OpenController : Controller + { + [AllowAnonymous] + public IActionResult Anonymous() => Ok(); + + public IActionResult RequiresLogin() => Ok(); + } + + private static DefaultHttpContext BuildContext(string path) + { + var context = new DefaultHttpContext(); + context.Request.Scheme = "https"; + context.Request.Host = new HostString("localhost:7157"); + context.Request.Path = new PathString(path); + return context; + } +} diff --git a/web/Classes/SitemapMiddleware.cs b/web/Classes/SitemapMiddleware.cs index 0ae70bbb1..22a4954a2 100644 --- a/web/Classes/SitemapMiddleware.cs +++ b/web/Classes/SitemapMiddleware.cs @@ -11,9 +11,11 @@ namespace Viper.Classes public class SitemapMiddleware { private readonly RequestDelegate _next; - public SitemapMiddleware(RequestDelegate next) + private readonly ILogger _logger; + public SitemapMiddleware(RequestDelegate next, ILogger logger) { _next = next; + _logger = logger; } public async Task Invoke(HttpContext context) @@ -36,37 +38,20 @@ public async Task Invoke(HttpContext context) { var methods = controller.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) .Where(method => typeof(IActionResult).IsAssignableFrom(method.ReturnType) || typeof(Task).IsAssignableFrom(method.ReturnType)) + .Where(IsPubliclyListable) .Distinct(); Dictionary URLs = new Dictionary(); foreach (var method in methods) { - Attribute? anonAttribute = method.GetCustomAttribute(typeof(AllowAnonymousAttribute)); - Attribute? anonAttributeClass = method.DeclaringType?.GetCustomAttribute(typeof(AllowAnonymousAttribute)); - Attribute? authAttribute = method.GetCustomAttribute(typeof(AuthorizeAttribute)); - Attribute? permAttribute = method.GetCustomAttribute(typeof(PermissionAttribute)); - Attribute? excludeAttribute = method.GetCustomAttribute(typeof(SearchExcludeAttribute)); - Attribute? excludeAttributeClass = method.DeclaringType?.GetCustomAttribute(typeof(SearchExcludeAttribute)); + string url = string.Format("{0}/{1}/{2}", rootUrl, controller.Name.ToLower().Replace("controller", ""), method.Name.ToLower()); + string lastMod = DateTime.UtcNow.ToString("yyyy-MM-dd"); - if (((anonAttribute != null // method is anonymous - || anonAttributeClass != null // or class is anonymous - ) - && (authAttribute == null // and method does not have authorize arrtribute - || permAttribute == null // or method does not have permission arrtribute - )) - && excludeAttribute == null && excludeAttributeClass == null) // and method and class do not have "search exclude" attribute + if (!URLs.ContainsKey(url)) { - string url = string.Format("{0}/{1}/{2}", rootUrl, controller.Name.ToLower().Replace("controller", ""), method.Name.ToLower()); - string lastMod = DateTime.UtcNow.ToString("yyyy-MM-dd"); - - if (!URLs.ContainsKey(url)) - { - URLs.Add(url, lastMod); - } - + URLs.Add(url, lastMod); } - } foreach (var url in URLs) { @@ -94,11 +79,13 @@ public async Task Invoke(HttpContext context) } // Middleware boundary: any sitemap-generation failure (DB, IO, // reflection, etc.) must fall through to the pipeline rather than - // break the request. + // break the request. Log it: swallowing silently is what let an + // AmbiguousMatchException turn the sitemap into a blanket 404 unnoticed. #pragma warning disable CA1031 - catch (Exception) + catch (Exception ex) #pragma warning restore CA1031 { + _logger.LogError(ex, "Sitemap generation failed; falling through to the pipeline."); await _next(context); } } @@ -107,6 +94,37 @@ public async Task Invoke(HttpContext context) await _next(context); } } + + /// + /// A sitemap advertises pages an anonymous visitor can actually reach, so an action + /// qualifies only when it is anonymous, ungated, and not search-excluded. + /// + /// + /// Every lookup covers the declaring controller as well as the method: [Permission] and + /// [SearchExclude] both target classes, and a class-level gate binds its actions. That + /// matters most for [Permission], which is an IAuthorizationFilter: only the built-in + /// AuthorizeFilter honors [AllowAnonymous], so a class-gated action still forbids at + /// runtime and must never be advertised. + /// + /// Testing [Permission] also covers [Authorize], because PermissionAttribute derives from + /// it. That inheritance is why these are plural lookups: a method carrying both + /// (HomeController.EmulateUser) matches AuthorizeAttribute twice, and the singular + /// GetCustomAttribute throws AmbiguousMatchException, which turned the sitemap into a 404. + /// + internal static bool IsPubliclyListable(MethodInfo method) + { + bool isAnonymous = HasAttribute(method); + bool isPermissionGated = HasAttribute(method); + bool isSearchExcluded = HasAttribute(method); + + return isAnonymous && !isPermissionGated && !isSearchExcluded; + } + + private static bool HasAttribute(MethodInfo method) where TAttribute : Attribute + { + return method.GetCustomAttributes(typeof(TAttribute), inherit: true).Length > 0 + || method.DeclaringType?.GetCustomAttributes(typeof(TAttribute), inherit: true).Length > 0; + } } public static class BuilderExtensions