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

 

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?

13 Comments

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



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

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

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

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

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

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

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

  • GerRedirectUrl issues an additional auth cookie.

  • 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?

  • 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?

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

  • 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

Comments have been disabled for this content.