dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.21k stars 9.95k forks source link

Expose error handler for Rewrite middleware #18342

Closed cpkuo closed 3 years ago

cpkuo commented 4 years ago

Would be nice to have an error handler when the Rewrite middleware raises an exception and have the ability to still call the next delegate. This would allow for logging any errors due to malformed rules and still execute the rest of the request.

analogrelay commented 4 years ago

This would allow for logging any errors due to malformed rules and still execute the rest of the request.

If the rules are malformed, the app is in an invalid state and really shouldn't continue executing. If you want this behavior, you should be able to create a custom rule (possibly wrapping existing rules) and handle this that way. Rewrite rules aren't really designed for this kind of recovery. If you want a rule that is resiliant to failure, the rule should avoid throwing.

Can you provide some examples of rules that throw exceptions which should be recovered from?

cpkuo commented 4 years ago

In my use case, I'm loading existing Apache mod rewrite rules. mod_rewrite handles errors itself and comes with its own logging for debugging of complex reg ex rules. This allows for dynamically created rules to be implemented without touching the RewriteOptions. Mistakes can be made in rules that currently can cause an exception when a rule is applied instead of logging the exception and gracefully continuing. In dynamic applications like a CMS in combination with a rule that "casts a wide net" can lead to many exceptions. Once such error I've come across is when the wrong back reference variable is used in the substitution part of the rule (% vs $).

analogrelay commented 4 years ago

Fair points, but I'm not sure it's something we'd prioritize building in to the product itself. There are a lot of scenarios that our rewrite middleware doesn't support, but in general it should have the building blocks you need to build the functionality you want.

For example, if you want to accomplish something like this now, you can wrap existing rules to make them suppress errors (and instead simply log and continue).

For example, something like this pseudo code would work:

var nestedOptions = new RewriteOptions()
    .AddApacheModRewrite(apacheModRewriteStreamReader);

var options = new RewriteOptions();
foreach(var rule in nestedOptions) {
    var wrappedRule = new ErrorSuppressingRule(rule);
    options.Add(wrappedRule);
}

app.UseRewriter(options);

Where ErrorSuppressingRule is defined as something like this:

public class ErrorSuppressingRule: IRule
{
    private IRule _innerRule;

    public ErrorSuppressingRule(IRule innerRule)
    {
        _innerRule = innerRule
    }

    public void ApplyRule(RewriteContext context)
    {
        try
        {
            _innerRule.ApplyRule(context);
        } catch(Exception ex)
        {
            // Log the error and continue
            context.Logger.LogError(ex, "Rule failed! Blah blah blah.");
            context.Result = RuleResult.ContinueRules;
        }
    }
}

With a little work you could definitely take this further and actually create a whole "ErrorSuppressing" extension method, something like

options = new RewriteOptions();
options.WithErrorSuppression(nestedOptions => {
    nestedOptions.AddApacheModRewrite(...);
    // Any rules you add here will be wrapped with "ErrorSuppressingRule"
});
cpkuo commented 4 years ago

I was able to use your example to catch errors however is there a way to retrieve detailed info on the IRule that threw the exception? For example, casting IRule to Microsoft.AspNetCore.Rewrite.ApacheModRewrite.ApacheModRewriteRule (which is marked internal) and logging the rule's reg ex pattern along with the current URL. Or perhaps including this information in the Exception. Otherwise I suppose I can use reflection to retrieve this information. Thanks!

analogrelay commented 4 years ago

There's no built-in way to do this. That would be true even if we added an event handler like you original requested though. At best we'd give you the IRule that failed, but you'd still end up in the same place.

analogrelay commented 4 years ago

I'm moving this to Discussions since we don't have action to take here. I'd be interested to know what, if any, errors you were getting because our rules generally shouldn't be throwing errors. There may be parsing errors which would be thrown at configuration time, but once the rules are parsed they shouldn't be throwing.

cpkuo commented 4 years ago

I came across one so far (in my limited testing) when attempting to use an existing .htaccess file. The file was initially updated to remove server variables not supported by the middleware and to use "^/" instead of just "^".

The rule below parses and can be added to the RewriteOption's rules but throws the exception at runtime.

RewriteRule ^/content/default.aspx$ https://www.somewhere.com$1 [NC,R=301,L]

backreference

This undoubtedly is a user error and from what I've seen, is a result of mistakenly using "$" instead of "%". I appreciate this middleware's ability to parse a .htaccess file but it does leave me excepting a few features from mod_rewrite, namely verbose error logging and a thread safe hot reload of the rules at runtime. But I do understand the statement in the documentation that other tools should be used with complex requirements.

analogrelay commented 4 years ago

The rule below parses and can be added to the RewriteOption's rules but throws the exception at runtime.

Ah, I see. It looks like there are indeed some cases where exceptions can be thrown. In this case, the regex can be correctly parsed, but fails to execute because of the missing backreference. I think this is by design since the rule is invalid. Ideally we'd throw on rule parsing (by detecting that the rule won't execute successfully). That might be quite tricky though.

ghost commented 3 years ago

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!