fix(auth): build CAS callbacks from a configured canonical origin - #310
fix(auth): build CAS callbacks from a configured canonical origin#310rlorenzo 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.88% +0.13%
==========================================
Files 992 993 +1
Lines 49697 49751 +54
Branches 5854 5866 +12
==========================================
+ Hits 20748 20839 +91
+ Misses 28038 27998 -40
- Partials 911 914 +3
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.
bee6b85 to
98e21f0
Compare
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
98e21f0 to
8c584e6
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
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 |
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)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: 2741 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).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:BaseUrlalready carries the same two URLs. I left it alone rather than widening this PR, but the two settings should probably converge later.