fix(auth): build CAS callbacks from a configured canonical origin - #310
Open
rlorenzo wants to merge 1 commit into
Open
fix(auth): build CAS callbacks from a configured canonical origin#310rlorenzo wants to merge 1 commit into
rlorenzo wants to merge 1 commit into
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
+ Coverage 41.74% 41.89% +0.14%
==========================================
Files 992 993 +1
Lines 49697 49762 +65
Branches 5854 5869 +15
==========================================
+ Hits 20748 20849 +101
+ Misses 28038 27998 -40
- Partials 911 915 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens CAS login/validation/logout callback URL generation by introducing a configured canonical public origin (instead of deriving the origin from the incoming request), reducing exposure to Host-header poisoning and aligning outbound URLs across the app.
Changes:
- Added
IPublicUrlService+ startup validation forApplication:PublicBaseUrl(fail-fast outside Development). - Updated CAS login/logout and
HttpHelper.GetRootURL()callers to use the canonical origin; fixed/2PathBase handling and strengthened/apiReturnUrl guarding. - Tightened
AllowedHostsin Test/Production and added unit tests covering canonical-origin behavior and validation rules.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Program.cs | Registers/validates PublicUrlOptions, wires IPublicUrlService into HttpHelper. |
| web/Controllers/HomeController.cs | Uses canonical base URL for CAS service/logout URLs; adjusts ReturnUrl handling and /api guard. |
| web/Classes/PublicUrlService.cs | New canonical-origin service + options + validator enforcing safe/expected base URL shapes. |
| web/Classes/HttpHelper.cs | Delegates GetRootURL() to IPublicUrlService (or request-derived fallback in Development). |
| web/appsettings.Test.json | Sets restrictive AllowedHosts and Application:PublicBaseUrl for Test. |
| web/appsettings.Production.json | Sets restrictive AllowedHosts and Application:PublicBaseUrl for Production. |
| test/Classes/PublicUrlServiceTests.cs | Unit tests for normalization, request fallback, and startup validation rules. |
| test/Classes/HomeControllerCasUrlTests.cs | Unit tests ensuring CAS URLs are built from configured origin and /api ReturnUrl yields 401. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rlorenzo
force-pushed
the
fix/cas-canonical-origin
branch
2 times, most recently
from
August 18, 2026 03:50
98e21f0 to
8c584e6
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdded a validated canonical public URL service. Integrated it with ChangesPublic URL integration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change builds CAS and related public URLs from validated canonical origins and preserves local development behavior. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant HomeController
participant IPublicUrlService
participant HttpHelper
HomeController->>IPublicUrlService: BuildUrl(relative redirect path)
IPublicUrlService-->>HomeController: return absolute public URL
HttpHelper->>IPublicUrlService: read BaseUrl or derive request URL
IPublicUrlService-->>HttpHelper: return application root URL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CAS login, ticket validation and logout derived their service URL from HttpHelper.GetRootURL(), which reads the request Host, and AllowedHosts was "*" in every environment. A Host header that got past the proxies could therefore poison a CAS callback. - Application:PublicBaseUrl per environment, validated on start so a deployed environment fails fast rather than falling back to the request - AllowedHosts narrowed to the real TEST/PROD hostnames plus localhost - GetRootURL() returns the canonical origin when configured, so the sitemap and emulation links stop being request-derived too - Login's /api guard strips the PathBase, so an API ReturnUrl gets a 401 instead of a CAS HTML redirect under the deployed /2 sub-app - Retire EmailSettings:BaseUrl, which held the same public origin under an email-specific name. Email links, the health-check collector and CAS now read one setting, so the two cannot drift
rlorenzo
force-pushed
the
fix/cas-canonical-origin
branch
from
August 18, 2026 21:16
8c584e6 to
5b9c031
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
web/Classes/HealthChecks/HealthCheckExtensions.cs:236
- HealthChecks UI endpoint URL reads Application:PublicBaseUrl directly but only trims a trailing '/'. This bypasses the whitespace + trailing-slash normalization you implemented in PublicUrlService/PublicUrlOptionsValidator (and can matter when the value comes from env vars/SSM with accidental whitespace), producing an invalid poll URL even though startup validation would accept the value.
var baseUrl = configuration["Application:PublicBaseUrl"]?.TrimEnd('/');
web/Classes/PublicUrlService.cs:156
- The scheme validation error message is correct for non-Development, but in Development it can trigger for non-http(s) schemes while still saying "outside Development", which is misleading during local setup/debugging. Consider making the message conditional so it accurately reflects the rules in each environment.
if (uri.Scheme != Uri.UriSchemeHttps && !(isDevelopment && uri.Scheme == Uri.UriSchemeHttp))
{
return ValidateOptionsResult.Fail($"{setting} must use https outside Development.");
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding
CAS login, ticket validation and logout all built their
serviceURL fromHttpHelper.GetRootURL(), which derives the origin fromRequest.GetDisplayUrl().AllowedHostswas"*"in every environment (no override in the Test or Production settings files). AHostheader that got past IIS/F5/Cloudflare could therefore poison a CAS callback URL. A restrictive CAS service registry limits the impact, but the application should not depend on that external control.Origins:
GetRootURL()in db9745e (2023-05-05), switched toGetDisplayUrl()in 24417f5 (2023-06-23); the CAS loginservicein dc63022 (2023-05-22) and 47ecde9 (2023-08-29);AllowedHosts: "*"in cf06887 (2023-05-03).Change
New
web/Classes/PublicUrlService.csPublicUrlOptionsbindsApplication:PublicBaseUrl.IPublicUrlService.BaseUrl/BuildUrl(path)supply the canonical origin.PublicUrlOptionsValidatorruns viaValidateOnStart()and fails startup outside Development when the value is missing, relative, non-https, or carries user-info, a query string, or a fragment.Configuration
Application:PublicBaseUrlAllowedHostshttps://secure-test.vetmed.ucdavis.edu/2secure-test.vetmed.ucdavis.edu;localhosthttps://viper.vetmed.ucdavis.edu/2viper.vetmed.ucdavis.edu;localhost*(unchanged)One setting, not two
EmailSettings:BaseUrlalready held exactly these URLs in Test and Production. Rather than add a second source of truth for the same fact, this PR removes it:EmailSettings.BaseUrlis deleted;ScheduleEditServiceandVerificationServiceinjectIPublicUrlServiceinstead.HealthCheckExtensionsreadsApplication:PublicBaseUrlfor the collector's poll URL.PostConfigurethat derived a base URL fromASPNETCORE_HTTPS_PORTmoves intoPublicUrlService, as the last-resort origin when there is no request to derive from (background email in dev). Deployed environments never reach it, because startup validation requires the configured value.Callers
HomeControllerinjectsIPublicUrlService;BuildRedirectUri, used by both the login redirect and thep3/serviceValidatecall, and the logoutservicenow come from configuration.HttpHelper.GetRootURL()delegates to the same service, so the sitemap and the Directory emulation link stop being request-derived as well. It also now usesRequest.PathBaseinstead of sniffing for a literal/2/prefix, which fixes the edge case where a request to exactly/2lost the base.Login'sreturnURL.StartsWith("/api")guard now strips the PathBase first. The SPAs sendReturnUrlalready prefixed (/2/api/...), so the guard never fired on TEST/PROD and an API caller got a CAS HTML redirect instead of a 401.Forwarded headers were already limited to
X-Forwarded-ForandX-Forwarded-Protowith the F5 and Cloudflare CIDRs as known proxies;X-Forwarded-Hostis still not trusted. No change needed there.Tests
test/Classes/PublicUrlServiceTests.csandtest/Classes/HomeControllerCasUrlTests.cs, 26 new tests:Host: attacker.exampleforBaseUrl,BuildUrl, the loginservice, and the logoutservice./2PathBase; an/apiReturnUrl returns 401 both with and without the PathBase prefix.Full backend suite: 2742 passed.
Deployment
Application:PublicBaseUrlin TEST and Production before deploying this code. It is in the checked-inappsettings.Test.json/appsettings.Production.json, so no SSM change is required, but confirm nothing overrides it. Startup fails with a clear message if it is missing.https://secure-test.vetmed.ucdavis.edu/2/CasLogin).Application:PublicBaseUrlinstead ofEmailSettings:BaseUrl. Effort verification email throws at send time if it is unset.AllowedHostsis now restrictive. If an F5 or IIS health probe reaches the app with a Host other than the canonical hostname orlocalhost(an IP or machine name, for example), it will be rejected. This is the one item I could not verify from the repo. If it shows up in the TEST logs, add that host to the list.Notes
EmailSettings:BaseUrlheld this same public origin under an email-specific name. It is now retired rather than left to drift: email links, the health-check collector and CAS all readApplication:PublicBaseUrl.