diff --git a/DESIGN.md b/DESIGN.md index a6210f794..5373e0f7e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -235,7 +235,7 @@ The splash sets white text over a darkened photograph, where a flat hex cannot a ## Typography -**Display and Brand Font:** Proxima Nova, the UC Davis campus typeface, self-hosted as woff2 at 400, 500, 700, and 800, with `-apple-system`, BlinkMacSystemFont, Segoe UI, Roboto, then Arial as fallbacks. Weight 900 downshifts to 800, since the family caps there. Font files carry a `?v=1` cache-buster because they are served with long-lived immutable headers; bump it in every stylesheet when a file is replaced. +**Display and Brand Font:** Proxima Nova, the UC Davis campus typeface, loaded as woff2 at 400, 500, 700, and 800 from the campus font server at `campusfont.ucdavis.edu`, with `-apple-system`, BlinkMacSystemFont, Segoe UI, Roboto, then Arial as fallbacks. Weight 900 downshifts to 800, since the family caps there. It may not be self-hosted: the campus license does not cover redistributing the files, which is what committing them to this repo would amount to. Three consequences, all expected. `campusfont.ucdavis.edu` must stay in the CSP `font-src` allowlist in `web/Program.cs`. The CDN returns `Access-Control-Allow-Origin` only to `*.ucdavis.edu` hosts, so on `localhost` the font fails CORS and the fallback stack renders instead; judge Proxima on TEST, not locally. And these URLs carry no `?v=` cache-buster, since campus owns their cache lifetime. **Body and Workspace Font:** Roboto, self-hosted as a variable woff2 covering weights 100 to 900, with the system sans stack behind it, extended with Apple Color Emoji, Segoe UI Emoji, and Noto Color Emoji so emoji render in the user's native set. **Icon Font:** Material Icons, self-hosted woff2, with a remote `fonts.gstatic.com` fallback declared in `site.css`. **Print-Only:** Ryman Eco is the UC Davis print display face and is deliberately not loaded on the web. Arial and Aptos are the brand-approved fallbacks when Proxima is unavailable. diff --git a/PRODUCT.md b/PRODUCT.md index 135bfbb46..096108244 100644 --- a/PRODUCT.md +++ b/PRODUCT.md @@ -54,7 +54,7 @@ The visual expression of all this, including what VIPER must not look like, is s Real assets that design work may rely on: -- **Proxima Nova**, the UC Davis campus typeface, self-hosted as woff2 at regular, medium, bold, and extrabold, in both `VueApp/src/assets/fonts/proxima-nova/` and `web/wwwroot/fonts/proxima-nova/`. +- **Proxima Nova**, the UC Davis campus typeface, at regular, medium, bold, and extrabold, loaded from the campus font server at `campusfont.ucdavis.edu`. It may not be self-hosted, so no copies live in this repo; the `@font-face` blocks in `web/wwwroot/css/site.css`, `welcome.css`, and `VueApp/src/styles/base.css` are the only references. - **Self-hosted Roboto and Material Icons**: `VueApp/src/assets/fonts/roboto-v51-latin.woff2`, `roboto-v51-latin-ext.woff2`, and `material-icons.woff2`, built to `web/wwwroot/vue/assets/`. - **Five login hero photographs** in AVIF and JPG at `web/wwwroot/images/login/`: guinea pig, horse and foal, ophthalmology, the SVM building, and vetmed admin. - **Brand marks**: the rod of asclepius (`rod-of-asclepius-white.avif` and `.png`), the `_ViperBrand.cshtml` lockup partial, `web/wwwroot/images/UCDSVMLogo.png`, and `nopic.jpg` as the person-photo placeholder. diff --git a/VueApp/src/CAHFS/pages/CAHFSAuth.vue b/VueApp/src/CAHFS/pages/CAHFSAuth.vue index 0feef73de..d5a68fe7a 100644 --- a/VueApp/src/CAHFS/pages/CAHFSAuth.vue +++ b/VueApp/src/CAHFS/pages/CAHFSAuth.vue @@ -1,5 +1,35 @@ @@ -36,54 +35,6 @@ const currentYear = new Date().getFullYear() height-hint="98" class="bg-white text-dark" > -
- UC Davis Veterinary Medicine logo -
- - - - - VIPER 2.0 - Development - Test - - -
-
{ diff --git a/VueApp/src/styles/base.css b/VueApp/src/styles/base.css index 50b1b1e2b..8cdf64aec 100644 --- a/VueApp/src/styles/base.css +++ b/VueApp/src/styles/base.css @@ -24,6 +24,46 @@ U+1E00-1E9F, U+1EF2-1EFF, U+2020, U+20A0-20AB, U+20AD-20C0, U+2113, U+2C60-2C7F, U+A720-A7FF; } +/* Proxima Nova, the UC Davis campus brand font for the blue header chrome. Loaded + from the campus font server per IET guidance rather than self-hosted, because + the license does not cover redistributing the files; that is also why these + are absolute URLs and not @/assets references for Vite to emit. Mirrors the + @font-face set in web/wwwroot/css/site.css, which carries the full rationale + and the CSP and CORS caveats. 900 is downshifted to 800 since the family caps + there. */ +@font-face { + font-family: "Proxima Nova"; + font-style: normal; + font-weight: 400; + font-display: swap; + src: url("https://campusfont.ucdavis.edu/proxima-nova/proximanova_regular_macroman/proximanova-regular-webfont.woff2") + format("woff2"); +} +@font-face { + font-family: "Proxima Nova"; + font-style: normal; + font-weight: 500; + font-display: swap; + src: url("https://campusfont.ucdavis.edu/proxima-nova/proximanova_medium_macroman/proximanova-medium-webfont.woff2") + format("woff2"); +} +@font-face { + font-family: "Proxima Nova"; + font-style: normal; + font-weight: 700; + font-display: swap; + src: url("https://campusfont.ucdavis.edu/proxima-nova/proximanova_bold_macroman/proximanova-bold-webfont.woff2") + format("woff2"); +} +@font-face { + font-family: "Proxima Nova"; + font-style: normal; + font-weight: 800; + font-display: swap; + src: url("https://campusfont.ucdavis.edu/proxima-nova/proximanova_extrabold_macroman/proximanova-extrabold-webfont.woff2") + format("woff2"); +} + /* Extend Quasar's body font-family cascade with OS emoji fonts so emoji characters render in color using the user's native emoji set (Apple on macOS/iOS, Segoe on Windows, Noto on Android/Linux). */ @@ -113,10 +153,58 @@ body { #mainLayoutHeader { background-color: var(--ucdavis-blue-100); /*background-color: white;*/ + /* Proxima Nova brand chrome for the blue bar; the gold section nav below + overrides back to Roboto. Medium (500) per UC Davis guidelines. */ + font-family: + "Proxima Nova", + -apple-system, + BlinkMacSystemFont, + "Segoe UI", + Roboto, + Arial, + sans-serif; + font-weight: 500; +} + +/* SVM brand lockup (gold rod-of-asclepius mark + school-name lockup). Mirrors + web/wwwroot/css/site.css .viper-brand* — keep the two in sync. */ +.viper-brand { + display: flex; + align-items: center; + gap: 0.75rem; +} + +.viper-brand__mark { + width: 2.75rem; + height: 2.75rem; + background-color: var(--ucdavis-gold-100); + display: flex; + align-items: center; + justify-content: center; + flex-shrink: 0; +} + +.viper-brand__mark img { + height: 2.5rem; /* nearly fills the 2.75rem mark, leaving a thin inset */ + width: auto; + display: block; +} + +.viper-brand__name { + height: 2.75rem; + width: auto; + display: block; +} + +/* Below lt-md the toolbar can't fit the wide lockup plus the nav controls and + profile, so keep only the rod mark. */ +@media (max-width: 1023.98px) { + .viper-brand__name { + display: none; + } } .mainLayoutViper { - font-family: Roboto; font-size: 1.6em !important; font-weight: normal; vertical-align: bottom; @@ -157,6 +245,16 @@ div.breadcrumbs { min-height: 36px; border-bottom: 1px solid silver; /*padding: 0 12px;*/ + /* Roboto here (overriding the header's Proxima) — narrower, so the many + section-nav items fit without crowding. */ + font-family: + Roboto, + -apple-system, + BlinkMacSystemFont, + "Segoe UI", + Arial, + sans-serif; + font-weight: 400; } #mainLayoutHeaderSections .selectedTopNav { @@ -167,11 +265,22 @@ div.breadcrumbs { #mainLayoutHeaderSections a.q-btn.navLink { } +/* Quasar's overlay-drawer glow bleeds a stray line down the left edge. */ +.q-drawer--mobile .q-layout__shadow:after { + box-shadow: none; +} + /*Left Nav styles*/ #leftNavMenu { overflow-x: hidden; } +/* Keep the menu clear of the floated close button so items aren't narrowed. */ +#leftNavMenu nav, +#leftNavMenu .q-list { + clear: both; +} + #leftNavMenu h2 { font-size: 1.2rem; margin: 0; diff --git a/test/Classes/HomeControllerCasUrlTests.cs b/test/Classes/HomeControllerCasUrlTests.cs new file mode 100644 index 000000000..335de4b6d --- /dev/null +++ b/test/Classes/HomeControllerCasUrlTests.cs @@ -0,0 +1,231 @@ +using System.Net; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Infrastructure; +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(), + 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 }; + + // IsSafeReturnUrl calls Url.IsLocalUrl, so give the controller a URL helper that + // mirrors framework semantics: rooted "/..." and app-relative "~/..." are local, + // protocol-relative ("//") and backslash ("/\\") forms are not. + var url = Substitute.For(); + url.IsLocalUrl(Arg.Any()).Returns(ci => + { + var candidate = ci.Arg(); + if (string.IsNullOrEmpty(candidate)) + { + return false; + } + + if (candidate.StartsWith('/')) + { + return !candidate.StartsWith("//") && !candidate.StartsWith("/\\"); + } + + return candidate.StartsWith("~/") + && !candidate.StartsWith("~//") + && !candidate.StartsWith("~/\\"); + }); + controller.Url = url; + + 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/Classes/SitemapMiddlewareTests.cs b/test/Classes/SitemapMiddlewareTests.cs new file mode 100644 index 000000000..acfdbfad1 --- /dev/null +++ b/test/Classes/SitemapMiddlewareTests.cs @@ -0,0 +1,92 @@ +using System.Text; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging.Abstractions; +using Viper.Classes; + +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); + } + + 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/test/Classes/Utilities/WelcomePageHelperTests.cs b/test/Classes/Utilities/WelcomePageHelperTests.cs new file mode 100644 index 000000000..299879240 --- /dev/null +++ b/test/Classes/Utilities/WelcomePageHelperTests.cs @@ -0,0 +1,48 @@ +using Viper.Classes.Utilities; + +namespace Viper.test.Classes.Utilities; + +public class WelcomePageHelperTests +{ + [Theory] + [InlineData(null, null)] + [InlineData("", null)] + [InlineData("/", null)] + [InlineData("/RAPS/Roles", "RAPS")] + [InlineData("/Effort/Foo", "Effort Reporting")] + [InlineData("/ClinicalScheduler/Schedule", "Clinical Scheduler")] + [InlineData("/CTS/Index", "Competency Tracking System")] + [InlineData("/Directory/Home", "Directory")] + [InlineData("/CMS/Page", "CMS")] + [InlineData("/Home/Policy", "Policy")] + [InlineData("/raps/roles", "RAPS")] + [InlineData("~/RAPS/Roles", "RAPS")] + [InlineData("~/Effort/Foo", "Effort Reporting")] + public void ResolveDestinationLabel_KnownAndFallbackCases(string? returnUrl, string? expected) + { + var actual = WelcomePageHelper.ResolveDestinationLabel(returnUrl); + Assert.Equal(expected, actual); + } + + [Theory] + [InlineData("http://evil.com")] + [InlineData("https://evil.com/path")] + [InlineData("//evil.com/x")] + [InlineData("//\\evil.com")] + [InlineData("~//evil.com")] + [InlineData("~/\\evil.com")] + [InlineData("javascript:alert(1)")] + [InlineData("RAPS/Roles")] + public void ResolveDestinationLabel_RejectsNonLocalUrls(string returnUrl) + { + Assert.Null(WelcomePageHelper.ResolveDestinationLabel(returnUrl)); + } + + [Theory] + [InlineData("/RAPS/Roles?id=5", "RAPS")] + [InlineData("/Effort/Foo#frag", "Effort Reporting")] + public void ResolveDestinationLabel_StripsQueryAndFragment(string returnUrl, string expected) + { + Assert.Equal(expected, WelcomePageHelper.ResolveDestinationLabel(returnUrl)); + } +} 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/Controllers/HomeControllerTests.cs b/test/Controllers/HomeControllerTests.cs new file mode 100644 index 000000000..30d139d83 --- /dev/null +++ b/test/Controllers/HomeControllerTests.cs @@ -0,0 +1,534 @@ +using System.Reflection; +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Controllers; +using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using NSubstitute; +using Viper.Classes.SQLContext; +using Viper.Classes; +using Viper.Controllers; +using Web.Authorization; + +namespace Viper.test.Controllers; + +/// +/// Unit tests for HomeController's anonymous landing / login flow, focused on the +/// open-redirect protections and redirect-loop guard added with the welcome page. +/// +public sealed class HomeControllerTests +{ + private readonly HomeController _controller; + + // Real area controllers, one per area. GetAreaNames() derives the area set from the + // "Viper.Areas..…" controller namespace, so these expose ClinicalScheduler/Effort/RAPS/CTS. + // Effort is intentionally an API-only area (its controllers carry no [Area]) — the case the + // namespace-based derivation fixes versus the old [Area] route-value lookup. + private static readonly Type[] _areaControllerTypes = + { + typeof(Viper.Areas.ClinicalScheduler.Controllers.CliniciansController), + typeof(Viper.Areas.Effort.Controllers.ReportsController), + typeof(Viper.Areas.RAPS.Controllers.RAPSController), + typeof(Viper.Areas.CTS.Controllers.CTSController), + }; + + // "/ClinicalScheduler" is a splash-eligible area landing page; "/ClinicalScheduler/rotation" is a deep link. + private static readonly string[] _areas = { "ClinicalScheduler", "Effort", "RAPS", "CTS" }; + + public HomeControllerTests() + { + var actionProvider = Substitute.For(); + var descriptors = _areaControllerTypes + .Select(t => new ControllerActionDescriptor { ControllerTypeInfo = t.GetTypeInfo() }) + .ToList(); + actionProvider.ActionDescriptors.Returns(new ActionDescriptorCollection(descriptors, version: 1)); + + _controller = new HomeController( + Substitute.For(), + Options.Create(new CasSettings { CasBaseUrl = "https://cas.example.edu/" }), + new PublicUrlService( + Options.Create(new PublicUrlOptions { PublicBaseUrl = "https://viper.example.edu/2" }), + Substitute.For()), + Substitute.For(), + Substitute.For(), + Substitute.For(), + actionProvider); + } + + /// + /// Wires up a controller context with the requested auth state and a URL helper whose + /// IsLocalUrl mirrors framework semantics (local = rooted path, not protocol-relative). + /// + private void Arrange(bool authenticated) + { + var identity = authenticated + ? new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "tester") }, authenticationType: "TestAuth") + : new ClaimsIdentity(); + + var httpContext = new DefaultHttpContext + { + User = new ClaimsPrincipal(identity), + RequestServices = new ServiceCollection().BuildServiceProvider(), + }; + httpContext.Request.Scheme = "https"; + httpContext.Request.Host = new HostString("viper.test"); + httpContext.Request.Path = "/login"; + + _controller.ControllerContext = new ControllerContext { HttpContext = httpContext }; + // View() resolves ITempDataDictionaryFactory from DI unless TempData is already set. + _controller.TempData = new TempDataDictionary(httpContext, Substitute.For()); + + var url = Substitute.For(); + url.IsLocalUrl(Arg.Any()).Returns(ci => + { + var candidate = ci.Arg(); + if (string.IsNullOrEmpty(candidate)) + { + return false; + } + + // Mirror framework semantics: rooted "/..." and app-relative "~/..." are + // local, but protocol-relative ("//"), backslash ("/\") and their "~/" + // variants are not. + if (candidate.StartsWith('/')) + { + return !candidate.StartsWith("//") && !candidate.StartsWith("/\\"); + } + + return candidate.StartsWith("~/") + && !candidate.StartsWith("~//") + && !candidate.StartsWith("~/\\"); + }); + _controller.Url = url; + } + + [Theory] + [InlineData(null, false)] + [InlineData("", false)] + [InlineData("/welcome", true)] + [InlineData("/Welcome", true)] + [InlineData("/welcome/", true)] + [InlineData("/login", true)] + [InlineData("/LOGIN?ReturnUrl=/x", true)] + [InlineData("/welcome#frag", true)] + [InlineData("/caslogin", true)] // re-entering the ticket handler without a ticket would 403 + [InlineData("/CasLogin/", true)] + [InlineData("/RAPS/Roles", false)] + [InlineData("/welcomepage", false)] + [InlineData("/caslogins", false)] + public void IsAuthEntryPath_DetectsLoopTargets(string? url, bool expected) + { + Assert.Equal(expected, HomeController.IsAuthEntryPath(url)); + } + + // Parity with the Vue guard, which rejects "../" and any "%2e" outright. A dot-segment survives + // IsLocalUrl and the root-relative /api check, then resolves somewhere else once the browser + // follows it — including in the percent-encoded spellings the URL spec also resolves. + [Theory] + [InlineData(null, false)] + [InlineData("", false)] + [InlineData("/Effort/Reports", false)] + [InlineData("/Effort/..api/x", false)] // ".." only counts as a whole segment + [InlineData("/dots../x", false)] + [InlineData("/Effort/%2ename/x", false)] // encoded dot only counts as a whole segment too + [InlineData("/Effort/../api/secret", true)] + [InlineData("/2/Effort/../api/secret", true)] + [InlineData("/./api/secret", true)] + [InlineData("/Effort/..", true)] + [InlineData("/Effort/../api?tab=1", true)] + [InlineData("/Effort/%2e%2e/api/secret", true)] // browsers resolve the encoded form the same way + [InlineData("/Effort/%2E%2E/api/secret", true)] // and the match is ASCII case-insensitive + [InlineData("/Effort/.%2e/api/secret", true)] // mixed encoding counts as ".." too + [InlineData("/Effort/%2e./api/secret", true)] + [InlineData("/%2e/api/secret", true)] + public void ContainsDotSegment_DetectsTraversal(string? url, bool expected) + { + Assert.Equal(expected, HomeController.ContainsDotSegment(url)); + } + + // The single ReturnUrl contract shared by /welcome, /login and /CasLogin, exercised under a + // subpath deployment so the base-prefixed and "~/" spellings are covered in one place. + [Theory] + [InlineData("/Effort", true)] + [InlineData("/2/Effort", true)] + [InlineData("~/Effort", true)] + [InlineData("/2/Effort/Reports?year=2026", true)] + [InlineData("/welcomepage", true)] // near-match on an entry point is a normal page + [InlineData(null, false)] + [InlineData("", false)] + [InlineData("https://evil.com/phish", false)] + [InlineData("//evil.com", false)] + [InlineData("/welcome", false)] // redirect loop + [InlineData("/login", false)] + [InlineData("/caslogin", false)] // ticketless re-entry 403s a user who just signed in + [InlineData("/2/welcome", false)] // base-prefixed entry points must be caught too + [InlineData("/2/CasLogin/", false)] + [InlineData("~/welcome", false)] // app-relative spelling must not slip past + [InlineData("/Effort/../api/secret", false)] + [InlineData("/2/Effort/%2e%2e/api/secret", false)] + public void IsSafeReturnUrl_EnforcesSharedContract(string? returnUrl, bool expected) + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + Assert.Equal(expected, _controller.IsSafeReturnUrl(returnUrl)); + } + + // Splash appears only for the front door (an empty return path or the bare site root) and for a + // single-segment area landing page. Anything deeper, or a single segment that is not a registered + // area, is treated as a deep link. + [Theory] + [InlineData(null, true)] + [InlineData("", true)] + [InlineData("/", true)] + [InlineData("/ClinicalScheduler", true)] + [InlineData("/clinicalscheduler", true)] // area match is case-insensitive + [InlineData("/Effort/", true)] // trailing slash on an area root still counts + [InlineData("/Effort?tab=1", true)] // query string is ignored + [InlineData("/ClinicalScheduler/rotation", false)] // deep link + [InlineData("/CTS/epa", false)] // CTS is an area, but this is a deep link + [InlineData("/MyPermissions", false)] // single segment, not an area + [InlineData("/cahfs", false)] // not a registered MVC area + public void IsSplashTarget_ClassifiesPassiveLandingsVsDeepLinks(string? url, bool expected) + { + var areas = new HashSet(_areas, StringComparer.OrdinalIgnoreCase); + Assert.Equal(expected, HomeController.IsSplashTarget(url, areas)); + } + + // In a subpath deployment (PathBase "/2") the ReturnUrl is prefixed with the base. StripPathBase + // removes it on a segment boundary so the splash classifier sees an app-relative path, while leaving + // unrelated paths (and the no-base dev case) untouched. + [Theory] + [InlineData(null, "/2", null)] + [InlineData("", "/2", "")] + [InlineData("/2/ClinicalScheduler", "/2", "/ClinicalScheduler")] + [InlineData("/2/ClinicalScheduler/rotation", "/2", "/ClinicalScheduler/rotation")] + [InlineData("/2", "/2", "")] // app root under the subpath + [InlineData("/2/", "/2", "/")] + [InlineData("/2?tab=1", "/2", "?tab=1")] // query is preserved for the classifier to strip + [InlineData("/22/x", "/2", "/22/x")] // segment boundary: "/2" must not strip from "/22" + [InlineData("/ClinicalScheduler", "", "/ClinicalScheduler")] // no base configured (dev) + [InlineData("/ClinicalScheduler", null, "/ClinicalScheduler")] + public void StripPathBase_RemovesBaseOnSegmentBoundary(string? url, string? pathBase, string? expected) + { + Assert.Equal(expected, HomeController.StripPathBase(url, pathBase)); + } + + // Area names come from controller namespaces (Viper.Areas..…), so API-only areas with no + // [Area] attribute are still recognized. Non-area namespaces and near-matches resolve to null. + [Theory] + [InlineData("Viper.Areas.Effort.Controllers", "Effort")] + [InlineData("Viper.Areas.ClinicalScheduler.Controllers.SomethingV2", "ClinicalScheduler")] + [InlineData("Viper.Areas.CMS", "CMS")] + [InlineData("Viper.Controllers", null)] // not an area + [InlineData("Viper.Areas", null)] // no area segment + [InlineData("Viper.AreasButNotReally.X", null)] // prefix must end on a namespace boundary + [InlineData(null, null)] + public void AreaFromControllerNamespace_ExtractsAreaSegment(string? ns, string? expected) + { + Assert.Equal(expected, HomeController.AreaFromControllerNamespace(ns)); + } + + [Fact] + public void Index_Anonymous_RendersWelcomeWithNoStore() + { + Arrange(authenticated: false); + + var result = _controller.Index(); + + var view = Assert.IsType(result); + Assert.Equal("Welcome", view.ViewName); + Assert.Equal("no-store,no-cache", _controller.Response.Headers.CacheControl.ToString()); + } + + // An area landing page ("/ClinicalScheduler") is a passive arrival, so it still gets the splash + // with its ReturnUrl preserved. + [Fact] + public void Welcome_Anonymous_AreaLanding_RendersSplashWithReturnUrl() + { + Arrange(authenticated: false); + + var result = _controller.Welcome("/ClinicalScheduler"); + + var view = Assert.IsType(result); + Assert.Equal("Welcome", view.ViewName); + Assert.Equal("/ClinicalScheduler", view.ViewData["ReturnUrl"]); + } + + // Effort is an API-only area (no [Area] MVC controller); its landing page must still splash. + // Regression guard for the namespace-based area derivation that replaced the [Area] route-value lookup. + [Fact] + public void Welcome_Anonymous_ApiOnlyAreaLanding_RendersSplash() + { + Arrange(authenticated: false); + + var result = _controller.Welcome("/Effort"); + + var view = Assert.IsType(result); + Assert.Equal("Welcome", view.ViewName); + Assert.Equal("/Effort", view.ViewData["ReturnUrl"]); + } + + // A deep link ("/ClinicalScheduler/rotation") skips the interstitial and goes straight to CAS + // via the Login action, carrying the ReturnUrl so the user lands where they intended. + [Fact] + public void Welcome_Anonymous_DeepLink_RedirectsToCasLogin() + { + Arrange(authenticated: false); + + var result = _controller.Welcome("/ClinicalScheduler/rotation"); + + var redirect = Assert.IsType(result); + Assert.Equal(nameof(HomeController.Login), redirect.ActionName); + Assert.Equal("/ClinicalScheduler/rotation", redirect.RouteValues?["ReturnUrl"]); + } + + // Under a subpath deployment (PathBase "/2") the area landing page arrives as "/2/ClinicalScheduler". + // The base is stripped for classification so it still gets the splash, with the friendly area label + // resolved and the full (base-prefixed) ReturnUrl preserved for the round trip. + [Fact] + public void Welcome_Anonymous_SubpathAreaLanding_RendersSplash() + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Welcome("/2/ClinicalScheduler"); + + var view = Assert.IsType(result); + Assert.Equal("Welcome", view.ViewName); + Assert.Equal("/2/ClinicalScheduler", view.ViewData["ReturnUrl"]); + Assert.Equal("Clinical Scheduler", view.ViewData["DestinationLabel"]); + } + + // A subpath deep link ("/2/ClinicalScheduler/rotation") still bypasses the splash and goes to CAS, + // carrying the full base-prefixed ReturnUrl. + [Fact] + public void Welcome_Anonymous_SubpathDeepLink_RedirectsToCasLogin() + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Welcome("/2/ClinicalScheduler/rotation"); + + var redirect = Assert.IsType(result); + Assert.Equal(nameof(HomeController.Login), redirect.ActionName); + Assert.Equal("/2/ClinicalScheduler/rotation", redirect.RouteValues?["ReturnUrl"]); + } + + // Under a subpath deployment every unsafe ReturnUrl arrives base-prefixed, so the base has to come + // off before the guards run. Both classes are covered here: auth entry points that would loop (or + // 403 on a ticketless re-entry), and dot-segments the browser resolves elsewhere after the CAS round + // trip. In each case the splash renders with a null ReturnUrl rather than bouncing back out to CAS. + [Theory] + [InlineData("/2/welcome")] + [InlineData("/2/login")] + [InlineData("/2/Welcome/")] + [InlineData("/2/caslogin")] // would re-enter the ticket handler ticketless and 403 after a good sign-in + [InlineData("/Effort/../api/secret")] + [InlineData("/2/Effort/../api/secret")] + [InlineData("/2/Effort/%2e%2e/api/secret")] // browsers resolve the encoded spelling the same way + public void Welcome_Anonymous_SubpathUnsafeReturnUrl_DropsReturnUrl(string returnUrl) + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Welcome(returnUrl); + + var view = Assert.IsType(result); + Assert.Equal("Welcome", view.ViewName); + Assert.Null(view.ViewData["ReturnUrl"]); + } + + // Authenticated welcome with no ReturnUrl under a subpath deployment redirects to "~/" so the app + // root keeps its PathBase ("/2/") instead of escaping to the domain root. Regression guard for the + // bare "/" that sent logged-in users out to the legacy site. + [Fact] + public void Welcome_Authenticated_NoReturnUrl_RedirectsToAppRelativeRoot() + { + Arrange(authenticated: true); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Welcome(); + + var redirect = Assert.IsType(result); + Assert.Equal("~/", redirect.Url); + } + + // Anonymous users still get the Welcome view, but any ReturnUrl that is non-local + // (open redirect) or points back at /welcome|/login (redirect loop) is dropped. + [Theory] + [InlineData("https://evil.com/phish")] + [InlineData("//evil.com")] + [InlineData("/welcome")] + [InlineData("/login")] + [InlineData("~/welcome")] // app-relative loop targets must still be caught after normalization + [InlineData("~/login")] + public void Welcome_Anonymous_DropsUnsafeReturnUrl(string returnUrl) + { + Arrange(authenticated: false); + + var result = _controller.Welcome(returnUrl); + + var view = Assert.IsType(result); + Assert.Null(view.ViewData["ReturnUrl"]); + } + + [Fact] + public void Welcome_Authenticated_RedirectsToLocalReturnUrl() + { + Arrange(authenticated: true); + + var result = _controller.Welcome("/Effort/Foo"); + + var redirect = Assert.IsType(result); + Assert.Equal("/Effort/Foo", redirect.Url); + } + + // App root is "~/" (not "/") so a subpath deployment keeps its PathBase ("/2/") rather than + // escaping to the domain root (the legacy site). + [Theory] + [InlineData("https://evil.com")] + [InlineData(null)] + public void Welcome_Authenticated_RedirectsToRootWhenReturnUrlInvalidOrMissing(string? returnUrl) + { + Arrange(authenticated: true); + + var result = _controller.Welcome(returnUrl); + + var redirect = Assert.IsType(result); + Assert.Equal("~/", redirect.Url); + } + + // The /api guard matches the api segment case-insensitively (routing is case-insensitive) and + // on a segment boundary, so "/api", "/api/...", "/api?..." are rejected in any casing while + // non-API paths that merely start with "api" (e.g. "/apiary") pass through to CAS. + [Theory] + [InlineData("/api", true)] + [InlineData("/api/foo", true)] + [InlineData("/API/foo", true)] + [InlineData("/Api?x=1", true)] + [InlineData("/api#frag", true)] + [InlineData("/apiary", false)] + [InlineData("/", false)] + public void IsApiPath_MatchesApiSegmentCaseInsensitivelyOnBoundary(string url, bool expected) + { + Assert.Equal(expected, HomeController.IsApiPath(url)); + } + + [Theory] + [InlineData("/api/secret")] + [InlineData("/API/secret")] // routing is case-insensitive, so the guard must be too + [InlineData("~/api/secret")] // app-relative form must not bypass the /api guard + public void Login_RejectsApiReturnUrl_WithUnauthorized(string returnUrl) + { + Arrange(authenticated: false); + + var result = _controller.Login(returnUrl); + + Assert.IsType(result); + } + + // A non-API path that merely starts with "api" is not caught by the guard; it proceeds to the + // normal CAS redirect. + [Fact] + public void Login_ForwardsNonApiPathStartingWithApiToCas() + { + Arrange(authenticated: false); + + var result = _controller.Login("/apiary"); + + Assert.IsType(result); + } + + // Under a subpath deployment the /api ReturnUrl arrives base-prefixed ("/2/api/..."). The base is + // stripped before the guard so it is still rejected and never forwarded to CAS. Regression guard for + // the pre-strip /api check that a "/2/api/..." ReturnUrl slipped past. + [Theory] + [InlineData("/2/api/secret")] + [InlineData("/2/API/secret")] // base-prefixed + mixed case must not bypass the guard + [InlineData("~/2/api/secret")] // app-relative + base-prefixed must not bypass the guard either + public void Login_RejectsSubpathApiReturnUrl_WithUnauthorized(string returnUrl) + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Login(returnUrl); + + Assert.IsType(result); + } + + [Theory] + [InlineData("https://evil.com/phish")] + [InlineData("//evil.com")] + public void Login_DoesNotForwardNonLocalReturnUrl(string returnUrl) + { + Arrange(authenticated: false); + + var result = _controller.Login(returnUrl); + + var redirect = Assert.IsType(result); + Assert.DoesNotContain("evil.com", redirect.Url, StringComparison.OrdinalIgnoreCase); + } + + // A dot-segment ReturnUrl passes IsLocalUrl and the root-relative /api check, but the browser + // resolves it after the CAS round trip: "/2/Effort/../api/secret" lands on "/2/api/secret" and + // dumps the user on a JSON 401. Dropped up front instead, matching the Vue guard. + // Asserting on "Effort" rather than "api/secret": the ReturnUrl is double URL-encoded into the CAS + // service URL, so any assertion containing a slash would pass even with the guard removed. + [Theory] + [InlineData("/Effort/../api/secret")] + [InlineData("/2/Effort/../api/secret")] + [InlineData("~/Effort/../api/secret")] + [InlineData("/2/Effort/%2e%2e/api/secret")] + public void Login_DoesNotForwardDotSegmentReturnUrl(string returnUrl) + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Login(returnUrl); + + var redirect = Assert.IsType(result); + Assert.DoesNotContain("Effort", redirect.Url, StringComparison.OrdinalIgnoreCase); + } + + // /login applies the same auth-entry guard as /welcome, so "/login?ReturnUrl=/welcome" cannot + // bounce the user back to the splash after a successful sign-in. ("/login" and "/caslogin" are + // covered by IsSafeReturnUrl above: both are substrings of the CAS service URL, so they can't be + // asserted against the redirect target here.) + [Theory] + [InlineData("/welcome")] + [InlineData("/2/welcome")] + [InlineData("~/welcome")] + public void Login_DoesNotForwardAuthEntryReturnUrl(string returnUrl) + { + Arrange(authenticated: false); + _controller.HttpContext.Request.PathBase = "/2"; + + var result = _controller.Login(returnUrl); + + var redirect = Assert.IsType(result); + Assert.DoesNotContain("welcome", redirect.Url, StringComparison.OrdinalIgnoreCase); + } + + // The anonymous "/" splash must carry the same no-store headers as /welcome, which gets them from + // [ResponseCache]. Both now route through the shared WelcomeSplash helper. + [Fact] + public void Index_And_Welcome_Anonymous_EmitMatchingNoStoreHeaders() + { + Arrange(authenticated: false); + _controller.Index(); + var indexCacheControl = _controller.Response.Headers["Cache-Control"].ToString(); + var indexPragma = _controller.Response.Headers["Pragma"].ToString(); + + Arrange(authenticated: false); + _controller.Welcome(); + + Assert.Equal("no-store,no-cache", indexCacheControl); + Assert.Equal("no-cache", indexPragma); + Assert.Equal(indexCacheControl, _controller.Response.Headers["Cache-Control"].ToString()); + Assert.Equal(indexPragma, _controller.Response.Headers["Pragma"].ToString()); + } +} 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/Classes/SitemapMiddleware.cs b/web/Classes/SitemapMiddleware.cs index 0ae70bbb1..dad30f650 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) @@ -42,20 +44,18 @@ public async Task Invoke(HttpContext context) 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)); + // Testing [Permission] alone covers [Authorize] too, because + // PermissionAttribute derives from it. That inheritance is also 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. + bool isAnonymous = method.GetCustomAttributes(typeof(AllowAnonymousAttribute), inherit: true).Length > 0 + || method.DeclaringType?.GetCustomAttributes(typeof(AllowAnonymousAttribute), inherit: true).Length > 0; + bool isPermissionGated = method.GetCustomAttributes(typeof(PermissionAttribute), inherit: true).Length > 0; + bool isSearchExcluded = method.GetCustomAttributes(typeof(SearchExcludeAttribute), inherit: true).Length > 0 + || method.DeclaringType?.GetCustomAttributes(typeof(SearchExcludeAttribute), inherit: true).Length > 0; - 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 (isAnonymous && !isPermissionGated && !isSearchExcluded) { string url = string.Format("{0}/{1}/{2}", rootUrl, controller.Name.ToLower().Replace("controller", ""), method.Name.ToLower()); string lastMod = DateTime.UtcNow.ToString("yyyy-MM-dd"); @@ -94,11 +94,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); } } diff --git a/web/Classes/Utilities/WelcomePageHelper.cs b/web/Classes/Utilities/WelcomePageHelper.cs new file mode 100644 index 000000000..daa287559 --- /dev/null +++ b/web/Classes/Utilities/WelcomePageHelper.cs @@ -0,0 +1,102 @@ +using System.Collections.Frozen; +using System.Globalization; + +namespace Viper.Classes.Utilities; + +/// +/// Pure helpers for the unauthenticated Welcome (landing) page. +/// +public static class WelcomePageHelper +{ + private static readonly FrozenDictionary AreaLabels = + new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["RAPS"] = "RAPS", + ["Effort"] = "Effort Reporting", + ["ClinicalScheduler"] = "Clinical Scheduler", + ["CTS"] = "Competency Tracking System", + ["Directory"] = "Directory", + ["CMS"] = "CMS", + }.ToFrozenDictionary(StringComparer.OrdinalIgnoreCase); + + /// + /// Resolve a human-readable destination label for the welcome page's deep-link indicator. + /// Returns null for missing, root-only, or non-local URLs. The check mirrors Url.IsLocalUrl semantics + /// so the helper is safe to call directly without prior validation. + /// + public static string? ResolveDestinationLabel(string? returnUrl) + { + if (!IsLocalUrl(returnUrl)) + { + return null; + } + + // After IsLocalUrl, returnUrl is non-null and non-empty. + var path = returnUrl!; + + int queryIndex = path.IndexOfAny(['?', '#']); + if (queryIndex >= 0) + { + path = path[..queryIndex]; + } + + // Strip a leading ~ (tilde-rooted app path) as well as /, so ~/Area resolves like /Area. + path = path.TrimStart('~', '/'); + if (path.Length == 0) + { + return null; + } + + var segments = path.Split('/', StringSplitOptions.RemoveEmptyEntries); + if (segments.Length == 0) + { + return null; + } + + if (AreaLabels.TryGetValue(segments[0], out var label)) + { + return label; + } + + return ToTitleCase(segments[^1]); + } + + /// + /// Defensive local-URL check matching Url.IsLocalUrl semantics. Rejects null/empty, absolute URLs + /// with a scheme, scheme-relative URLs (//host), and anything not beginning with a single '/'. + /// + private static bool IsLocalUrl(string? url) + { + if (string.IsNullOrEmpty(url)) + { + return false; + } + + if (url[0] == '/') + { + // Reject scheme-relative URLs like //evil.com/x and //\evil.com + if (url.Length == 1) + { + return true; + } + return url[1] != '/' && url[1] != '\\'; + } + + if (url[0] == '~' && url.Length > 1 && url[1] == '/') + { + // Reject ~// and ~/\ for parity with Url.IsLocalUrl semantics + if (url.Length == 2) + { + return true; + } + return url[2] != '/' && url[2] != '\\'; + } + + return false; + } + + private static string ToTitleCase(string value) + { + return char.ToUpper(value[0], CultureInfo.InvariantCulture) + value[1..]; + } +} diff --git a/web/Controllers/HomeController.cs b/web/Controllers/HomeController.cs index 1f7d613b7..d4607adaf 100644 --- a/web/Controllers/HomeController.cs +++ b/web/Controllers/HomeController.cs @@ -8,9 +8,10 @@ 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.Controllers; using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Options; using Viper.Areas.CMS.Data; @@ -36,17 +37,21 @@ 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; + private readonly IActionDescriptorCollectionProvider _actionDescriptorProvider; - 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, IActionDescriptorCollectionProvider actionDescriptorProvider) { this._clientFactory = clientFactory; this._settings = settingsOptions.Value; + this._publicUrl = publicUrl; this._aAUDContext = aAUDContext; this._rapsContext = rapsContext; this._viperContext = viperContext; this._userHelper = new UserHelper(); + this._actionDescriptorProvider = actionDescriptorProvider; } /// /// VIPER 2 home page @@ -56,9 +61,269 @@ public HomeController(IHttpClientFactory clientFactory, IOptions se [SearchName(FriendlyName = "Viper 2 Homepage")] public IActionResult Index() { + if (User.Identity?.IsAuthenticated != true) + { + // Anonymous splash served in-place at "/". The authenticated home response + // below keeps its default caching. + return WelcomeSplash(returnUrl: null, destinationLabel: null); + } return View(); } + /// + /// Unauthenticated landing/splash page. Anonymous users see the welcome splash; + /// authenticated users are redirected to the validated ReturnUrl or "/". + /// + [Route("/[action]")] + [AllowAnonymous] + [SearchExclude] + [ResponseCache(NoStore = true, Location = ResponseCacheLocation.None)] +#pragma warning disable S6967 // Action only reads ReturnUrl, no model binding required + public IActionResult Welcome([FromQuery] string? ReturnUrl = null) +#pragma warning restore S6967 + { + // Normalize "~/..." to "/..." (mirrors Login) so the loop-guard catches + // ~/welcome and ~/login and we never emit a "~/" redirect target. + ReturnUrl = NormalizeAppRelativeUrl(ReturnUrl); + + // In a subpath deployment the ReturnUrl carries the PathBase (e.g. "/2/ClinicalScheduler"), + // so strip it once here — the classifier and label resolver both need it root-relative. + // The full ReturnUrl is preserved for the redirect/links back. + var relativeReturnUrl = StripPathBase(ReturnUrl, Request.PathBase.Value); + + if (!IsSafeReturnUrl(ReturnUrl)) + { + ReturnUrl = null; + relativeReturnUrl = null; + } + + if (User.Identity?.IsAuthenticated == true) + { + // "~/" (not "/") so the app root keeps the PathBase ("/2/") in a subpath deployment + // instead of redirecting out to the domain root (the legacy site). + return LocalRedirect(string.IsNullOrEmpty(ReturnUrl) ? "~/" : ReturnUrl); + } + + // Only passive arrivals get the splash: the bare site root or a top-level area + // landing page (e.g. "/ClinicalScheduler"). A deep link (e.g. "/ClinicalScheduler/rotation") + // skips the interstitial and goes straight to CAS so we don't interrupt a targeted workflow. + if (!IsSplashTarget(relativeReturnUrl, GetAreaNames(_actionDescriptorProvider))) + { + return RedirectToAction(nameof(Login), new { ReturnUrl }); + } + + return WelcomeSplash(ReturnUrl, WelcomePageHelper.ResolveDestinationLabel(relativeReturnUrl)); + } + + // Single owner of the splash's ViewData and cache-header contract, shared by /welcome and by + // the anonymous "/" landing. Welcome carries [ResponseCache(NoStore, Location=None)]; Index + // has no such attribute, so the headers are set here to keep the two responses identical. + private IActionResult WelcomeSplash(string? returnUrl, string? destinationLabel) + { + Response.Headers["Cache-Control"] = "no-store,no-cache"; + Response.Headers["Pragma"] = "no-cache"; + + ViewData["ReturnUrl"] = returnUrl; + ViewData["Hero"] = PickRandomHeroKey(); + ViewData["DestinationLabel"] = destinationLabel; + + return View("Welcome"); + } + + private static readonly string[] _heroKeys = + { + "svm_building", + "vetmed_admin", + "ophthalmology", + "guinea_pig", + "horse_foal", + }; + + private static string PickRandomHeroKey() + { + return _heroKeys[Random.Shared.Next(_heroKeys.Length)]; + } + + // 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; + + // The ReturnUrl contract shared by every auth entry point (/welcome, /login, /CasLogin), so the + // three cannot drift apart: the URL must be local, must not point back at an auth entry point, + // and must not carry a dot-segment. Normalizes and strips the PathBase internally, since the + // path guards below all compare root-relative paths. + // internal (not private) so the shared guard is unit-testable via InternalsVisibleTo. + internal bool IsSafeReturnUrl(string? returnUrl) + { + if (!Url.IsLocalUrl(returnUrl)) + { + return false; + } + + var path = StripPathBase(NormalizeAppRelativeUrl(returnUrl), Request.PathBase.Value); + return !IsAuthEntryPath(path) && !ContainsDotSegment(path); + } + + // Everything before the query or fragment: the ReturnUrl guards all classify on the path alone. + private static string PathWithoutQuery(string url) + { + int cut = url.IndexOfAny(['?', '#']); + return cut >= 0 ? url[..cut] : url; + } + + // The auth entry points, which must never be a ReturnUrl: /welcome and /login would + // redirect-loop, and /caslogin would re-enter the ticket handler without a ticket and + // 403 a user who just signed in successfully. + private static readonly string[] _authEntryPaths = ["/welcome", "/login", "/caslogin"]; + + // internal (not private) so the redirect-loop guard is unit-testable via InternalsVisibleTo. + internal static bool IsAuthEntryPath(string? url) + { + if (string.IsNullOrEmpty(url)) + { + return false; + } + + var path = PathWithoutQuery(url).TrimEnd('/'); + + return _authEntryPaths.Contains(path, StringComparer.OrdinalIgnoreCase); + } + + // Browsers resolve dot-segments before issuing the request, and the URL spec counts the + // percent-encoded spellings too: "%2e" is ".", and ".%2e"/"%2e."/"%2e%2e" are "..", all + // ASCII case-insensitive. + private static readonly string[] _dotSegments = [".", "..", "%2e", "%2e%2e", ".%2e", "%2e."]; + + // The Vue guard rejects "../" and any "%2e" outright (RequireLogin.ts). Match it here: a + // ReturnUrl like "/Effort/../api/x" passes IsLocalUrl and the root-relative /api check, but the + // browser resolves it to "/api/x" after the CAS round trip and dumps the user on a JSON 401. + // internal (not private) so it is unit-testable via InternalsVisibleTo. + internal static bool ContainsDotSegment(string? url) + { + if (string.IsNullOrEmpty(url)) + { + return false; + } + + return PathWithoutQuery(url) + .Split('/') + .Any(segment => _dotSegments.Contains(segment, StringComparer.OrdinalIgnoreCase)); + } + + // Controllers under web/Areas live in the "Viper.Areas..…" namespace. Deriving the area + // set from controller namespaces (rather than the [Area] route value) covers every area — + // including SPA areas whose controllers are API-only and carry no [Area] attribute — and needs + // no hand-maintained list: add an area the usual way and it is picked up automatically. + private const string AreaNamespacePrefix = "Viper.Areas."; + + // Reference type (not a tuple) so the field assignment below is atomic: a multi-word struct + // could be read torn by a concurrent request mid-write. + private sealed record AreaNameCache(ActionDescriptorCollection Source, HashSet Areas); + + // Cached per descriptor collection: the collection is immutable and replaced wholesale + // (new instance) only when endpoints change, so the area set is derived once instead of + // per anonymous /welcome request. Benign race: concurrent first requests may each compute + // the set; last writer wins with an identical result. + private static AreaNameCache? _areaNamesCache; + + // The set of top-level area names (e.g. "Effort", "ClinicalScheduler"). Used to tell an area + // landing page ("/Effort" → splash) apart from a deep link ("/Effort/Reports" → CAS). + private static HashSet GetAreaNames(IActionDescriptorCollectionProvider actionDescriptorProvider) + { + var descriptors = actionDescriptorProvider.ActionDescriptors; + var cache = _areaNamesCache; + if (cache == null || !ReferenceEquals(cache.Source, descriptors)) + { + var areas = descriptors.Items + .OfType() + .Select(d => AreaFromControllerNamespace(d.ControllerTypeInfo.Namespace)) + .Where(area => area != null) + .Select(area => area!) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + cache = new AreaNameCache(descriptors, areas); + _areaNamesCache = cache; + } + + return cache.Areas; + } + + // Extracts the area segment from a controller namespace, e.g. "Viper.Areas.Effort.Controllers" + // → "Effort". Returns null for non-area namespaces. internal so it is unit-testable. + internal static string? AreaFromControllerNamespace(string? ns) + { + if (ns == null || !ns.StartsWith(AreaNamespacePrefix, StringComparison.Ordinal)) + { + return null; + } + + var rest = ns[AreaNamespacePrefix.Length..]; + int dot = rest.IndexOf('.'); + var area = dot >= 0 ? rest[..dot] : rest; + return area.Length == 0 ? null : area; + } + + // The welcome splash is reserved for passive arrivals: the bare site root or a top-level + // area landing page (a single path segment matching a registered area). Anything deeper is + // a deep link that should bypass the interstitial. Null/empty ReturnUrl is the front door. + // internal (not private) so the classifier is unit-testable via InternalsVisibleTo. + internal static bool IsSplashTarget(string? url, ISet areaNames) + { + if (string.IsNullOrEmpty(url)) + { + return true; + } + + var path = PathWithoutQuery(url).Trim('/'); + + if (path.Length == 0) + { + return true; + } + + if (path.Contains('/')) + { + return false; + } + + return areaNames.Contains(path); + } + + // 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]/")] [Authorize(Policy = "2faAuthentication")] [Permission(Allow = "SVMSecure")] @@ -94,16 +359,31 @@ 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, ""); + // Normalize app-relative "~/..." to "/..." before validating, so the + // /api guard below cannot be bypassed and we never forward an invalid + // browser URL to CAS. + ReturnUrl = NormalizeAppRelativeUrl(ReturnUrl); + + if (!IsSafeReturnUrl(ReturnUrl)) + { + ReturnUrl = null; + } + + // The application root under the deployed PathBase ("" locally, "/2" on TEST/PROD). + // Read from the request rather than derived from GetRootURL(), which now returns the + // configured canonical origin and so no longer cancels against the request authority. + 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(); } @@ -260,7 +540,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); } @@ -284,16 +564,15 @@ public IActionResult MyPermissions() return View(); } - - /// - /// 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); } /// @@ -344,7 +623,16 @@ private async Task AuthenticateCasLogin(string? ticket, string? r var user = new ClaimsPrincipal(claimsIdentity); await HttpContext.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, user); - return new LocalRedirectResult(!String.IsNullOrWhiteSpace(returnUrl) ? returnUrl : "/"); + // Same contract as /welcome and /login. This is the redirect the browser actually + // follows after CAS, so a ReturnUrl that arrived via a hand-crafted service URL + // (bypassing those two) is dropped here too. + if (!IsSafeReturnUrl(returnUrl)) + { + returnUrl = null; + } + + // "~/" (not "/") so a subpath deployment ("/2") lands on the app root, not the domain root. + return new LocalRedirectResult(!String.IsNullOrWhiteSpace(returnUrl) ? returnUrl : "~/"); } } catch (TaskCanceledException ex) diff --git a/web/Program.cs b/web/Program.cs index 126ab15c8..261d84c2a 100644 --- a/web/Program.cs +++ b/web/Program.cs @@ -141,7 +141,7 @@ .AddCookie(options => { options.Cookie.Name = "VIPER.Authentication.UCD"; - options.LoginPath = new PathString("/login"); + options.LoginPath = new PathString("/welcome"); options.AccessDeniedPath = new PathString("/Error/403"); options.ExpireTimeSpan = TimeSpan.FromHours(12); }); @@ -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(); @@ -357,8 +349,8 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db // Allow fonts to be downloaded from: csp.AllowFonts - .FromSelf()// This domain - .From("fonts.gstatic.com"); + .FromSelf() // Roboto and Material Icons, self-hosted under /fonts + .From("https://campusfont.ucdavis.edu"); // Proxima Nova - campus license forbids self-hosting // Allow other sites to put this in an iframe? csp.AllowFraming @@ -382,7 +374,6 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db // Allow styles csp.AllowStyles .FromSelf() // This domain - .From("fonts.googleapis.com") // Google Fonts stylesheets .AllowUnsafeInline(); // Allows inline CSS })); @@ -437,6 +428,20 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db RedirectToAppendTrailingSlash = true }); + // Self-hosted fonts (Roboto, Material Icons), served with long-lived cache + // headers. Proxima Nova is not here: it loads from campusfont.ucdavis.edu, + // since the campus license does not allow us to host the files ourselves. + app.UseStaticFiles(new StaticFileOptions + { + FileProvider = new PhysicalFileProvider( + Path.Join(builder.Environment.WebRootPath, "fonts")), + RequestPath = "/fonts", + OnPrepareResponse = ctx => + { + ctx.Context.Response.Headers["Cache-Control"] = "public, max-age=31536000, immutable"; // 1 year + } + }); + // General static files (favicon, /css, /js, /images, etc.). app.UseStaticFiles(); @@ -526,7 +531,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/Views/Home/Welcome.cshtml b/web/Views/Home/Welcome.cshtml new file mode 100644 index 000000000..45529af04 --- /dev/null +++ b/web/Views/Home/Welcome.cshtml @@ -0,0 +1,101 @@ +@{ + Layout = null; + // Controller has already validated ReturnUrl with Url.IsLocalUrl and nulled it if unsafe. + var returnUrl = ViewData["ReturnUrl"] as string; + var loginUrl = string.IsNullOrEmpty(returnUrl) + ? Url.Content("~/login") + : $"{Url.Content("~/login")}?ReturnUrl={Uri.EscapeDataString(returnUrl)}"; + var hero = ViewData["Hero"] as string ?? "ophthalmology"; + var destinationLabel = ViewData["DestinationLabel"] as string; + var year = DateTime.Now.Year; + // Preload the AVIF that the CSS image-set() picks on capable browsers (must + // match that URL byte-for-byte to be reused). The type hint below lets + // browsers without AVIF skip the preload and fall back to the JPEG. + var heroImageUrl = Url.Content($"~/images/login/photo-{hero.Replace("_", "-")}.avif"); +} + + + + + + Sign in - VIPER 2.0 + @* Proxima Nova comes from the campus font server (see welcome.css), so warm + the connection early: the splash headline is display-scale and the + handshake would otherwise land in the middle of the font-display swap. *@ + + + + + +
+
+ +
+ +
+ +
+
+
+

VIPER 2.0

+ +

+ The SVM portal for students, staff, and faculty. +

+
+ +
+
+

Welcome

+ @if (!string.IsNullOrEmpty(destinationLabel)) + { +

+ You'll be taken to @destinationLabel after signing in. +

+ } + + Sign in + + + + Need help signing in? + (opens in new window) + +
+
+
+ + +
+ + diff --git a/web/Views/Shared/Components/ProfilePic/Default.cshtml b/web/Views/Shared/Components/ProfilePic/Default.cshtml index ddabbefc2..b57915ebd 100644 --- a/web/Views/Shared/Components/ProfilePic/Default.cshtml +++ b/web/Views/Shared/Components/ProfilePic/Default.cshtml @@ -1,4 +1,4 @@ -@model Viper.Models.AAUD.AaudUser +@model Viper.Models.AAUD.AaudUser @if (Model != null) { IUserHelper UserHelper = new UserHelper(); @@ -45,7 +45,7 @@ } else { - + Login diff --git a/web/Views/Shared/_VIPERLayout.cshtml b/web/Views/Shared/_VIPERLayout.cshtml index 805e6f629..d15f21f89 100644 --- a/web/Views/Shared/_VIPERLayout.cshtml +++ b/web/Views/Shared/_VIPERLayout.cshtml @@ -11,6 +11,9 @@ @await RenderSectionAsync("Metatags", required: false) @ViewData["Title"] - VIPER(2.0) + @* Proxima Nova comes from the campus font server (see site.css); warm the + connection before the stylesheet asks for it. *@ + @if (HttpHelper.HttpContext != null && HttpHelper.HttpContext.Request.Path.ToString().ToLower().Contains("/directory")) { @@ -25,7 +28,6 @@ } -