Aspnetcore: Regex Timeout are too short, not consistent, and not configurable in RewriteMiddleware

[jkotalik]

Regex timeouts that are used across UrlRewrite, ApacheModRewrite, and code rules are not consistent. The are set at 1 second or 1 millisecond in different places. Secondly, 1 millisecond is too short for a regex expression; it should be 1 second as the timeout is mostly for making sure your server isn't locked. Also if the expression timeouts, it returns a 500 server error as the exception is unhandled. Thirdly, the timeout should be configurable in RewriteOptions.

Plan of action:

• Timeouts should be increased to 1 second across the board.
• For 2.1, we should make Regex timeouts configurable a
• The timeouts should be backported to 2.0 and 1.1

@Eilon @muratg this is a patch candidate. Workarounds are very difficult.

[Eilon]

Need to consult with @blowdart on a good timeout. I agree 1ms is waaaay too small.

[blowdart]

1.1 milliseconds.

Merry Christmas ⛄️

Oh ok, 1 second is fine as a default, as long as it's configurable.

[Eilon]

@blowdart but in a patch, we might not make it configurable. Would 1sec be reasonable in the patch? (With a switch to restore the original bizarre 1ms timeout.)

[jkotalik]

The patch is a simple fix. However the redesign would require more work. When calling options.AddRedirect(), options.AddIISUrlRewrite, etc, we eagerly create the Regex object, which is too early as options would now store the regex timeout. Someone could do:

options.AddRedirect(...); // Creates a regex object with default timeout of 1 ms
options.RegexTimeout = TimeSpan.FromSeconds(1);
options.AddRewrite(...);

which would set the timeout of 1 ms instead of 1 second for the regex expression. We shouldn't have options apis that need to be ordered.
We could either create the regex objects on first request or use a different pattern to know when rules are done being added.

[blowdart]

Config would be better, but if we can't do that for a patch, then, ugh, ok fix it at 1 second.

[Eilon]

@jkotalik yeah I don't think having a property on the options would be the right way to do it. It would have to be a parameter on each .AddXyz() call.

[Tratcher]

options.RegexTimeout = TimeSpan.FromSeconds(1); looks like a simple, piratical solution. Changing it to DefaultRegexTimeout might make it's usage a little more clear. The ordering restriction is not a hard thing to explain to users.

Passing in the timeout to every .Add API looks like overkill, you'll add a lot of overloads that very few people will use, and they'll have to specify it on every rule.

[jkotalik]

Adding timeouts through each of the .AddXyz() calls would be bloated for sure. I guess we can add it to options and throw if someone does:

options.AddRule();
options.RegexTimeout = TimeSpan.FromSeconds(1); // throw

[Eilon]

Or don't throw and make it work: have every call to .AddXyz() capture the "current" RegexTimeout and use it.

[jkotalik]

280 for an initial fix to dev. Will make PRs to 1.1.x and 2.0.x branches and then address fixes to 2.1.x.

[Eilon]

FYI, we ended up deciding no to patch this. But for 2.1 we can consider if we want some fancier behavior(s).