DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.5k stars 350 forks source link

System.ArgumentOutOfRangeException in DynamicSchemeAuthenticationMiddleware #1171

Closed vitalyobukhov closed 1 year ago

vitalyobukhov commented 1 year ago

Which version of Duende IdentityServer are you using? 6.1.0

Which version of .NET are you using? net6.0

Describe the bug Duende.IdentityServer.Hosting.DynamicProviders.DynamicSchemeAuthenticationMiddleware throws System.ArgumentOutOfRangeException in case if /scheme part was not provided within the URI of request.

To Reproduce Just plug in the standard IdentityServer via IdentityServerApplicationBuilderExtensions.UseIdentityServer within the web app and try to access /federation path without trailing slash or scheme-specific suffix.

Expected behavior No exception - avoid context.Request.Path.Value.Substring(startIndex + 1) if startIndex >= context.Request.Path.Value.Length.

Log output/exception with stacktrace

System.ArgumentOutOfRangeException
startIndex cannot be larger than length of string. (Parameter 'startIndex')
   at System.String.Substring(Int32 startIndex, Int32 length)
   at Duende.IdentityServer.Hosting.DynamicProviders.DynamicSchemeAuthenticationMiddleware.Invoke(HttpContext context) in /_/src/IdentityServer/Hosting/DynamicProviders/DynamicSchemes/DynamicSchemeAuthenticationMiddleware.cs:line 48

Additional context Found it during ISEs 500 triage.

josephdecock commented 1 year ago

Apart from not throwing, is your expectation that the dynamic providers middleware would ignore routes like these?

vitalyobukhov commented 1 year ago

I think there're two paths to improve it:

  1. Treat it as a valid match which miss a required parameter (scheme). An appropriate exception would be fine then.
  2. Treat it as 'not found' since the request doesn't even has a trailing slash and pass the flow to next middleware.

I'm ok with both. The main point why I raised the issue is that we want to exclude security scans from our logs in a nice way.

brockallen commented 1 year ago

PR submitted for this, @vitalyobukhov.