Home > .NET, Fun, General Software Development, Software Development > Can you find the bug in this code?

Can you find the bug in this code?

This is a real bug that I came across yesterday in some code I had written about a week before.  I was a little surprised at the mechanics but it makes sense once you understand what is happening …

   1:  private void Foo()
   2:  {
   3:      try
   4:      {
   5:          AuthenticationInfo authenticationInfo = GetAuthenticationInfo(Request.QueryString["t"]);
   6:          Authenticator authenticator = new Authenticator(new LoginProvider());
   7:          AuthenticationStatus authenticationStatus = authenticator.Authenticate(authenticationInfo);
   8:          if (authenticationStatus.Authenticated)
   9:          {
  10:              IUser user =
  11:                  BLUser.Load(authenticationInfo.UserName, authenticationInfo.OrganizationCode, authenticationInfo.DomainId);
  12:              user.SetNewSessionId();
  13:              AuthenticationTokenParser authenticationTokenParser = new AuthenticationTokenParser();
  14:              string authenticationToken = authenticationTokenParser.Create(user.UserName, user.OrganizationId, user.DomainId);
  15:              FormsAuthentication.SetAuthCookie(authenticationToken, true);
  16:              string redirectUrl = FormsAuthentication.GetRedirectUrl(authenticationToken, true);
  17:              if (redirectUrl == null || redirectUrl.Trim().Length == 0)
  18:              {
  19:                  redirectUrl = "~/Home.aspx";
  20:              }
  21:              Response.Redirect(redirectUrl, true);
  22:          }
  23:          Response.Redirect("~/Home.aspx");
  24:      }
  25:      catch
  26:      {
  27:          Response.Redirect("~/Home.aspx");
  28:      }
  29:  }

.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

 

I will post the answer soon if no-one gets it. 🙂

 

Jonathan Cogley is the CEO and founder of Thycotic Software, a .NET consulting company and ISV in Washington DC.  Our product, Secret Server is a enterprise password manager system for teams to secure their passwords.  Is your team still storing passwords in a text file?

  1. http://
    March 25, 2008 at 12:54 pm

    I can’t see all of the code. It’s getting cut off on the right side.

    I’m using Internet Explorer 7 on Windows XP SP2.

  2. http://
    March 25, 2008 at 1:07 pm

    1: private void Foo()

    2: {

    3: try

    4: {

    5: AuthenticationInfo authenticationInfo = GetAuthenticationInfo(Request.QueryString[“t”]);

    6: Authenticator authenticator = new Authenticator(new LoginProvider());

    7: AuthenticationStatus authenticationStatus = authenticator.Authenticate(authenticationInfo);

    8: if (authenticationStatus.Authenticated)

    9: {

    10: IUser user =

    11: BLUser.Load(authenticationInfo.UserName, authenticationInfo.OrganizationCode, authenticationInfo.DomainId);

    12: user.SetNewSessionId();

    13: AuthenticationTokenParser authenticationTokenParser = new AuthenticationTokenParser();

    14: string authenticationToken = authenticationTokenParser.Create(user.UserName, user.OrganizationId, user.DomainId);

    15: FormsAuthentication.SetAuthCookie(authenticationToken, true);

    16: string redirectUrl = FormsAuthentication.GetRedirectUrl(authenticationToken, true);

    17: if (redirectUrl == null || redirectUrl.Trim().Length == 0)

    18: {

    19: redirectUrl = “~/Home.aspx”;

    20: }

    21: Response.Redirect(redirectUrl, true);

    22: }

    23: Response.Redirect(“~/Home.aspx”);

    24: }

    25: catch

    26: {

    27: Response.Redirect(“~/Home.aspx”);

    28: }

    29: }

  3. thycotic
    March 25, 2008 at 1:14 pm

    @drakiula: Thanks – seems to be something with the new blog theme causing the code to be cut off.

  4. http://
    March 25, 2008 at 1:17 pm

    Wild guess, line 23 should have been inside else {}

  5. http://
    March 25, 2008 at 1:46 pm

    Probally not what you mean, but “~/Home.aspx” can be found 3 times in the same method, which means it’s a 100% candidate for refactoring into a const and the “Response.Redirect” could be located under the closing bracket from the catch. 🙂

  6. http://
    March 25, 2008 at 1:48 pm

    Response.Redirect will throw a ThreadAbortException causing the catch block to be executed.

    The solution is to call the overload of Response.Redirect(string url, bool endresponse) with endresponse set to *false*. This prevents it from raising the ThreadAbortException. Keep in mind that this causes the entire page to execute.

    We ran into this issue with ASP.net 1.0. I believe 1.1 is when the overload for Response.Redirect got introduced.

  7. Basgun
    March 25, 2008 at 1:58 pm

    I don’t know your default filename settings, but I would refactor the code (from 17 to 23) as:

    if (redirectUrl != null)
    {
    Response.Redirect(redirectUrl, true);
    }
    else
    {
    Response.Redirect(“~/Home.aspx”);
    }

    redirectUrl might be just an empty string (“”) in order to redirect to the default page (perhaps Default.aspx).

  8. cibrax
    March 25, 2008 at 2:41 pm

    I am not completely sure, probably the Response.Redirect(redirectUrl, true); is throwing an AbortedThread exception and it is being caught in the generic handler. In the end, the generic handler is redirecting the user to a completely different page (Home.aspx).

    Regards,
    Pablo.

  9. http://
    March 25, 2008 at 2:48 pm

    I have to agree with rajbk. Redirecting forces the page to say, “Hey, I know you’re still doing something, but I am redirecting.” in the form of an exception. which should take you to the catch block and force you to home.aspx every time.

  10. http://
    March 25, 2008 at 3:28 pm

    GerRedirectUrl issues an additional auth cookie.

  11. http://
    March 25, 2008 at 4:15 pm

    If the code were arranged in three blocks: the 1st to authenticate, the 2nd to use that information to create a url, and the 3rd to redirect to that url.

    Then, maybe, this issue could be avoided?

  12. http://
    March 26, 2008 at 12:03 am

    The threadabort exception has already been mentioned but something else I noticed is unless a redirectUrl is specified the user will be redirected to home.aspx. Assuming Home.aspx is a page that requires the user to be authenticated the user is now stuck in an infinite login -> redirect to login page loop.

    Also is there any particular reason for not using String.IsNullorEmpty on line 17?

  13. Hosam Kamel
    March 26, 2008 at 7:24 am

    Missing Object null values check , the AuthenticationInformation may be null to due missing querystring …

  14. thycotic
    March 26, 2008 at 10:32 pm

    Thanks to everyone for your responses!

    I have posted the “fix” here:

    http://weblogs.asp.net/jcogley/archive/2008/03/26/can-you-find-the-bug-in-this-code-fixed.aspx

  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: