Invalid characters in the request URL causes an ArgumentException to be raised

[brentnewbury]

A request with invalid characters (e.g. /%3C [edit] which is URL decoded to <) causes an ArgumentException to be raised. This produces an HTTP 500 error to be returned to the client, rather than the expected HTTP 404.

Below if the stack trace from the exception:

ArgumentException: Illegal characters in path.
System.IO.Path.CheckInvalidPathChars(String path, Boolean checkAdditional)
System.IO.Path.GetExtension(String path)
Microsoft.AspNet.StaticFiles.FileExtensionContentTypeProvider.TryGetContentType(String subpath, String& contentType)
Microsoft.AspNet.StaticFiles.StaticFileContext.LookupContentType()
Microsoft.AspNet.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)

[brentnewbury]

Wondering if we should check for invalid path characters as part of the StaticFileContext.ValidatePath() method call in StaticFileMiddleware.Invoke(HttpContext context)? I would expect a method called ValidatePath to do something appropriate with invalid characters in the path, i.e. return false in this case.

The odd thing is the log output would be "The request path {Path} does not match the path filter". I have an feeling I might have misunderstood the purpose of the method due to the log output if it returns false and the comment // Check if the URL matches any expected paths

[brentnewbury]

I wanted to take a whack at fixing this issue, but I think the solution might warrant some discussion.

For example, do we strip invalid characters from the request in the static file middleware? Do we check for invalid characters in ValidatePath(), return false if we find any, and change the error message outputted by the middleware after the call? Do we add a new method which checks for invalid characters, which the static file middleware calls, outputting an appropriate log entry?

I'd want to try and avoid sticking try/catches around the methods as we get a lot of first chance exceptions happening, potentially pausing the debugger, etc. Invalid characters aren’t exceptional, we should try to detect the problem and handle it gracefully.

[brentnewbury]

Let me know if that potential solution is good enough for a pull request. I've renamed the existing ValidatePath() method to MatchPath() as that's what it's doing. Then added a new ValidatePath() method that will check for invalid characters (and potentially other path validation in the future).

[Tratcher]

Related: aspnet/FileSystem#147

[brentnewbury]

Just looked through that issue @Tratcher, and I agree with @davidfowl; they're kinda related in that they're both trying to deal with odd characters, but I think both issues need their separate fixes. Let me know if https://github.com/brentnewbury/StaticFiles/commit/11e02d1a513f41a8dc704027dd6a50e25f64054e seems like a good candidate for a pull request and I'll create one.

[davidfowl]

I'd want to try and avoid sticking try/catches around the methods as we get a lot of first chance exceptions happening, potentially pausing the debugger, etc. Invalid characters aren’t exceptional, we should try to detect the problem and handle it gracefully.

Yes!

@brentnewbury do send a pull request for your change. The only thing I'm not sure about is if the static file handler should pass through to the next middleware. I could see that being ok though.

[brentnewbury]

I let it pass through so that other middleware can try to handle it, better to passthrough than to swallow it up. Maybe in the future, if it's asked for, we could make it optional. I'll submit that pr now.