Can you find the bug in this code?

Published Tuesday, March 25, 2008 8:04 AM

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:  }

 

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?

Comments

# Dwayne said on Tuesday, March 25, 2008 8:54 AM

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.

# drakiula said on Tuesday, March 25, 2008 9:07 AM

  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:  }

# thycotic said on Tuesday, March 25, 2008 9:14 AM

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

# drakiula said on Tuesday, March 25, 2008 9:17 AM

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

# JV said on Tuesday, March 25, 2008 9:46 AM

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. :)

# rajbk said on Tuesday, March 25, 2008 9:48 AM

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.

# Basgun said on Tuesday, March 25, 2008 9:58 AM

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).

# cibrax said on Tuesday, March 25, 2008 10:41 AM

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.

# Lee said on Tuesday, March 25, 2008 10:48 AM

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.

# mhildreth said on Tuesday, March 25, 2008 11:28 AM

GerRedirectUrl issues an additional auth cookie.

# Thomas Eyde said on Tuesday, March 25, 2008 12: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?

# Zack Jones said on Tuesday, March 25, 2008 8:03 PM

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?

# Hosam Kamel said on Wednesday, March 26, 2008 3:24 AM

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

# thycotic said on Wednesday, March 26, 2008 6:32 PM

Thanks to everyone for your responses!  

I have posted the "fix" here:

weblogs.asp.net/.../can-you-find-the-bug-in-this-code-fixed.aspx

This Blog

Syndication