Redirect to a page with endResponse to true VS CompleteRequest and security thread

Question

Base on this questions and the answers there, I like to ask what is the proper way of redirecting.

The default way using the Redirect(url, endResponse) is throw the ThreadAbortException because is called with endResponse=true that calls the End() method and so, if you use it inside a try/catch block, this exception shown there and that can be assumed as error, but actually a user is try to redirect to a page by stopping the rest of the page processing.

The other possible ways is to call the Redirect(url, endResponse) with endResponse=false following by HttpContext.Current.ApplicationInstance.CompleteRequest(); By using that you do not get any exceptions.

So the question is what is better to use and why.

Solution 1

You must call the redirect always with endRespose=true or else any hacker can see whats on the page by simple hold the redirect.

To prove that I use the NoRedirect plugin for Firefox to hold the redirect. Then I test the two cases and here is the results:

I have a simple page with that text inside it

<form id="form1" runat="server">
<div>
I am making a redirect - you must NOT see this text.
</div>
</form>

and then on Page load try to make the redirect with both cases:

First case, using the Complete Request();

    try
    {
        // redirect with false that did not throw exception
        Response.Redirect("SecondEndPage.aspx", false);
        // complete the Request
        HttpContext.Current.ApplicationInstance.CompleteRequest();
    }
    catch (Exception x)
    {

    }

and there boom, you can see whats inside the page !

And second case

try
{
    // this is throw the ThreadAbortException exception
    Response.Redirect("SecondEndPage.aspx", true);
}
catch (ThreadAbortException)
{
    // ignore it because we know that come from the redirect
}
catch (Exception x)
{

}

Nothing shown now.

So if you do not like a hacker to see whats on your page, you must call it with endResponse to true and stop what other processing is made -eg return from function and not continue.

If for example you check if the user is authenticated he can see that page or if not he must redirect to login, and even in the login if you try to redirect him with endResponse to false, then holding the redirect the hacker can see - what you believe that can not because you use the Redirect.

My basic point here is to show the security thread that exist if you are not stop to send data back to the browser. The redirect is a header and instruction to the browser, but at the same time you need to stop send any other data, you must stop send any other part of your page.

Solution 2

It is not required to call Response.Redirect with true for endResponse to solve the security issue of outputting the page content after the redirect call. You can accomplish this another way and avoid causing a ThreadAbortException at the same time (which is always bad). Below are snippets of a page I created with 5 buttons that cause redirects in different ways, the RedirectRenderOverride button being the ideal as it is the one that triggers the Render method to do nothing. This has been tested with the NoRedirect add-in. Only two cases avoid outputting anything other than the 302 object moved response - RedirectEnd and RedirectRenderOverride.

Code In Front

<asp:Button ID="Button1" runat="server" OnClick="RedirectCompleteRequest" Text="RedirectCompleteRequest"/>
<asp:Button ID="Button2" runat="server" OnClick="RedirectClear" Text="RedirectClear"/>
<asp:Button ID="Button3" runat="server" OnClick="RedirectRenderOverride" Text="RedirectRenderOverride"/>
<asp:Button ID="Button4" runat="server" OnClick="RedirectEnd" Text="RedirectEnd"/>
<asp:Button ID="Button5" runat="server" OnClick="RedirectEndInTryCatch" Text="RedirectEndInTryCatch"/>

Code Behind

public partial class _Default : Page {
    private bool _isTerminating;

    protected void RedirectEnd(object sender, EventArgs e) { Response.Redirect("Redirected.aspx"); }

    protected void RedirectCompleteRequest(object sender, EventArgs e)
    {
        Response.Redirect("Redirected.aspx", false);
        HttpContext.Current.ApplicationInstance.CompleteRequest();
    }

    protected void RedirectClear(object sender, EventArgs e)
    {
        Response.Clear();
        Response.Redirect("Redirected.aspx", false);
    }

    protected void RedirectRenderOverride(object sender, EventArgs e)
    {
        Response.Redirect("Redirected.aspx", false);
        _isTerminating = true;
    }

    protected void RedirectEndInTryCatch(object sender, EventArgs e)
    {
        try {
            Response.Redirect("Redirected.aspx");
        } catch (ThreadAbortException) {
            // eat it
        } finally {
            Response.Write("Still doing stuff!");
        }
    }

    protected override void RaisePostBackEvent(IPostBackEventHandler sourceControl, string eventArgument)
    {
        if (!_isTerminating) {
            base.RaisePostBackEvent(sourceControl, eventArgument);
        }
    }

    protected override void Render(HtmlTextWriter writer)
    {
        if (!_isTerminating) {
            base.Render(writer);
        }
    }
}

Response.End calls Thread.CurrentThread.Abort internally and, according to Eric Lippert, calling Thread.Abort, "is at best indicative of bad design, possibly unreliable, and extremely dangerous."

Comments

[Aristos]

This is a totally different approach, compare it with your answer here stackoverflow.com/a/14640542/159270 where you do not explain the issue and the security thread !

[Sumo]

I do mention the approach and provide a link stating, "To avoid postback handling and HTML rendering, you need to do more: web.archive.org/web/20101224113858/http://www.c6software.com‌​/…"

[Aristos]

I read the article and the only argument I see is: is not good to throw exceptions. You say- you need to do more, and give a link that have other links...

[Aristos]

you have fun :) you place a Respnse.Write inside the finally hehehe, the finally is used to clear some object, not to write on page. :) Ok you have fun.

[Sumo]

Didn't say it was good code. It proves a point that the ThreadAbortException and the thread is really not dead yet, nor may never be. You make a valid security point which folks need to think about. But, if Microsoft is documenting the fact that you should never call it, another approach must be found.

[Aristos]

The Response.Write("Still doing stuff!"); is always render direct on the page, the Render function of yours did not stop it. I just test it. So when you won to stop something you responsible to stop and the rest of the render actions. Now the finally must run to clear/close some object, that is the reason to use it.

[Sumo]

That's correct, because the _isTerminating is false. The overrides in that code example are only being used when _isTerminating is set to true, which only happens in the RedirectRenderOverride event handler.

[Aristos]

you do not understand, the Response.Write is send direct to the browser, is not pass from the render. Is not be used on code behind or you break the page.

[Sumo]

Again, correct. However, there are 5 distinct examples in this code example. They are not to be mixed together. The Response.Write in that part of the example of something that can happen where you think you've aborted the thread to do the redirect. It's not part of the Render override example.

[Aristos]

This code is a nice idea, to stop the rendering - but I say you again, you do not point that out on your answers when you say to the users to avoid to use the endRequest with true - and you place them in a security thread just because they do not understand what is this exception and thy afraid of it.

[Aristos]

if you negative vote, please consider to say what is the issue with this approach.

[Dai]

Downvoted (not least because "hacker" is inappropriate terminology) but because Microsoft themselves say to not use endResponse: true or Response.Redirect(String). Code that throws ThreadAbortException is bad anyway for many reasons I won't get into. The solution is simple: use Response.SuppressContent = true (and ideally, don't render anything instead of suppressing rendered content to save wasted CPU time).

[Aristos]

@Dai Thank you for the comments - I will find some times to make some tests with the SuppressContent (and I will update the answer). About the is bad anyway for many reasons I won't get into, if you have some article to read about, or give me one of that many reasons to consider them. Thank you again for your replay.

[Dai]

See blogs.msdn.microsoft.com/tmarq/2009/06/25/… and docs.microsoft.com/en-us/aspnet/aspnet/overview/…