apereo / dotnet-cas-client

Apereo .NET CAS Client
Apache License 2.0
234 stars 172 forks source link

[AllowHtml] attribute #102

Open mcamp1 opened 4 years ago

mcamp1 commented 4 years ago

I'm running into a problem using the AllowHtml attribute to allow posting HTML strings with DotNetCasClient installed. Any help would be appreciated. The attribute works fine without the CAS client, but throws an error as soon as it is installed.

Model: [AllowHtml] public string Password { get; set; }

Stack Trace: Server Error in '/' Application. A potentially dangerous Request.Form value was detected from the client (Password="<strong>Hello World<..."). Description: ASP.NET has detected data in the request that is potentially dangerous because it might include HTML markup or script. The data might represent an attempt to compromise the security of your application, such as a cross-site scripting attack. If this type of input is appropriate in your application, you can include code in a web page to explicitly allow it. For more information, see http://go.microsoft.com/fwlink/?LinkID=212874. Exception Details: System.Web.HttpRequestValidationException: A potentially dangerous Request.Form value was detected from the client (Password="<strong>Hello World<..."). Source Error: An unhandled exception was generated during the execution of the current web request. Information regarding the origin and location of the exception can be identified using the exception stack trace below. Stack Trace: [HttpRequestValidationException (0x80004005): A potentially dangerous Request.Form value was detected from the client (Password="<strong>Hello World<...").] System.Web.HttpRequest.ValidateString(String value, String collectionKey, RequestValidationSource requestCollection) +9906997 System.Web.<>c__DisplayClass280_0.<ValidateHttpValueCollection>b__0(String key, String value) +23 System.Web.HttpValueCollection.EnsureKeyValidated(String key) +9904951 System.Web.HttpValueCollection.GetValues(Int32 index) +30 System.Collections.Specialized.NameValueCollection.Add(NameValueCollection c) +84 System.Web.HttpRequest.FillInParamsCollection() +52 System.Web.HttpRequest.GetParams() +90 System.Web.HttpRequest.get_Params() +33 DotNetCasClient.Utils.RequestEvaluator.GetRequestIsCasSingleSignOut() +100 DotNetCasClient.CasAuthenticationModule.OnBeginRequest(Object sender, EventArgs e) +288 System.Web.SyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +144 System.Web.HttpApplication.ExecuteStepImpl(IExecutionStep step) +50 System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +73 Version Information: Microsoft .NET Framework Version:4.0.30319; ASP.NET Version:4.8.4001.0

phantomtypist commented 4 years ago

Can I get you to post up a sample app where you run into this problem? (e.g. in a temporary repo and link to it from here?)

I'd just like some sort of reproduction scenario.

mcamp1 commented 4 years ago

I created a sample app at https://github.com/mcamp1/AllowHtml/

Thanks!

GitHub
mcamp1/AllowHtml
CAS validation reproduction scenario. Contribute to mcamp1/AllowHtml development by creating an account on GitHub.
TheHokieCoder commented 4 years ago

@mcamp1 Sorry for the long time since you have posted and not getting back to you. Are you still experiencing an issue with your form input, or have you found a workaround?

mcamp1 commented 4 years ago

Thanks for checking in! I am still experiencing the issue, and the only workaround I've found is to just disable validation. Any help would be appreciated!

TheHokieCoder commented 4 years ago

@mcamp1 Thanks for confirming that. I was looking through the code yesterday and see where the .NET CAS client is pulling values out of POST-ed data using the .NET Framework's helper method, which passes through the forms validation and causes the exception to occur. You are applying the [AllowHTML] attribute to a single property, and I am guessing that the attribute is not yet processed (in the request pipeline) when the .NET CAS client is reading a form value.

What I think needs to happen is the .NET CAS client needs to access the form values via the non-validated collection so that it never triggers the protection of .NET forms validation. Instead, it will manually perform any validation on the values it needs, and that way it won't step on the toes of what your application is (or isn't) doing with forms validation. I will test this out and create a fix for you if my assumption is correct.

phantomtypist commented 4 years ago

@TheHokieCoder let me know if the automated building on AppVeyor fails. I think it may run into a problem and I'll need to fix it. If you do a push against the repo in the develop branch (gitflow methodology) or trigger a PR, AppVeyor should do an automatic build against the Cake build script in the repo.

TheHokieCoder commented 4 years ago

@phantomtypist Thanks for that heads up!

Partial good news is that, for .NET v4.5 and onwards, I was able to fix the request validation errors that the OP was encountering. v4.5+ provides a collection of non-validated inputs that don't trigger validation exceptions (HttpRequest.Unvalidated). However, before v4.5, such as v4.0, there is no such collection. Based on my research in documentation, it appears that the only recourse is to set the request validation mode back to 2.0 so that validation is only done for page requests...not all requests. (I found some useful info on an OWASP site) This is not ideal, but as far as I can tell there is no facility to read request inputs (query string and form) without triggering the request validation AND with leaving the validation mode at 4.0. Unless someone else can prove me wrong, I think we'll need to fix this bug for .NET v4.5+ and then make a note in the documentation that the request validation mode needs to be set back to 2.0 in order to prevent the exceptions.

Another option is to swallow those exceptions and log them, but there are two cons to this approach:

  1. Users would see odd behavior, such as a valid CAS authentication taking place, "logging in" would fail due to the exception, and the user would be asked to authenticate again. The only way to tell what is going on would be to look at the logs. I don't see this to be in any way better that getting upfront exceptions.
  2. Logging the exceptions would require some serious overhaul of the RequestEvaluator class as it currently does not have a way to pass in a logger instance. It is basically a static collection of helper methods. Or, we would have to overhaul any location in the CAS handler that calls those methods to catch the exceptions there.

I am OK with the originally proposed solution to throw up the caveat about .NET v4.0 and fix it for v4.5+. But I would like to get further input and give someone the chance to prove me wrong by showing a way we can read query string and form values without triggering validation in .NET Framework v4.0.

phantomtypist commented 4 years ago

I'm in favor of just doing the implementation in .NET 4.5.x and fully sunsetting the .NET 4.0 and lower support in this project. At this point in time, people need to stop using the older versions of the framework. It's going to make things easier in the long run internally within the project too.

phantomtypist commented 4 years ago

Any objections from anyone deprecating support for anything lower than .NET 4.5.x?

TheHokieCoder commented 4 years ago

@phantomtypist How about we:

  1. Update the docs for v1.X of this client with information about this "known issue" and how to work around it (bumping the validation mode down to 2.0).
  2. Drop support for .NET Framework < v4.5.2, fixing the request validator issue, and bumping the version up to v2.0 to semantically indicate a breaking change?
  3. Declare a policy that v1.X will no longer be supported and that moving forward only versions of .NET Framework officially supported by Microsoft will be supported by this client. Per Microsoft's product lifecycle search page, v4.0, v4.5.0, and v4.5.1 of the framework are officially not supported. Therefore, I believe that our base target should be v4.5.2.
  4. Close any issues related to those unsupported frameworks (if any exist)